Add a donate button to the navbar #6600
Conversation
|
Yay, another button in the navbar, and there's already a community thread asking for a link to community there as well.. Seriously though if you want to propose a link to the donation site then just do that - framing the site is just going to be horrible. Also please don't call commits "first iteration..." even if you do think they might change - any PR you open should in principle be ready to merge so the commit description just describe it as if it was final. If this had been good to merge I would have wanted to be able to do so without having to come back to ask for the commit message to be updated. |
1ec5
left a comment
1ec5
left a comment
There was a problem hiding this comment.
These changes make the existing donation link redundant. The link in map attribution is already obscure, so removing it shouldn’t break anyone’s muscle memory too bad.
openstreetmap-website/config/layers.yml
Lines 7 to 10 in 7fdee46
Without any additional attribution string, the attribution code needs to be optional:
openstreetmap-website/app/assets/javascripts/leaflet.map.js
Lines 62 to 64 in 7fdee46
Yay, another button in the navbar, and there's already a community thread asking for a link to community there as well..
There’s some discussion in #6517 and #6575 about opportunities to consolidate existing navbar items so we don’t end up with a confusingly long list. Even if the navbar isn’t the perfect place for a donation link, it’s still a much better place than an attribution line that only sometimes appears on the map.
| <%= link_to t("layouts.help"), help_path, :class => header_nav_link_class(help_path) %> | ||
| </li> | ||
| <li class="nav-item"> | ||
| <%= link_to t("javascripts.map.make_a_donation"), donate_path, :class => header_nav_link_class(donate_path) %> |
There was a problem hiding this comment.
The localizable string no longer has anything to do with map JavaScript, so we should replace it with one right next to the “Help” string in en.yml.
| @@ -0,0 +1,3 @@ | |||
| <% content_for :content do %> | |||
| <%= tag.iframe "", :frameBorder => 0, :id => "donate-embed", :class => "w-100 vh-100", :autofocus => true, :src => t("site.copyright.legal_babble.lead_2_making_donation_url") %> | |||
There was a problem hiding this comment.
Should the donation page URL even be localizable? I think there’s only one version of the page, which should be hard-coded or moved to config/settings.yml and referenced through Settings.
| @@ -0,0 +1,3 @@ | |||
| <% content_for :content do %> | |||
| <%= tag.iframe "", :frameBorder => 0, :id => "donate-embed", :class => "w-100 vh-100", :autofocus => true, :src => t("site.copyright.legal_babble.lead_2_making_donation_url") %> | |||
There was a problem hiding this comment.
Seriously though if you want to propose a link to the donation site then just do that - framing the site is just going to be horrible.
The iframe is probably in response to the concern in #3041 (comment) about taking the user away from the main site’s navigation, making it harder for the user to come back. On the other hand, the supporting site’s homepage isn’t optimized for being embedded in this site. There’s also at least one link to OSM that will cause this site to nest within itself.
There was a problem hiding this comment.
the user away from the main site’s navigation
By the way, it is not clear why this is bad. Just open the link in a new tab. Right now, clicking on most of the buttons reloads the page, which is also distracting, but doesn't bother anyone.
There was a problem hiding this comment.
Opening it in a new tab is also an option, probably even cleaner than this iframe design. We might want to indicate that it opens a new tab, using either a discreet icon or a tooltip.
Then the link to the copyright is also redundant, because it is in the header. But seriously, you forget that the buttons in the header are sometimes hidden, for example, on mobile devices or if the user uses vertical tabs. Also in other languages, the header width is larger than in the English localization. You can deal with duplicate links after this PR. |
I pointed that out in #6575 (comment). 🙂
The Donate link will end up in the overflow menu in a narrow window or in the collapsible header on a mobile screen. I suspect the Donate link would still be more discoverable there than in the attribution line. The attribution link only appears on the homepage and only if the Standard or Shortbread layer is active. The link doesn’t have to always be visible, but it does need to be accessible without taking unrelated steps. That said, I agree that adding the link to the navigation bar is the more impactful change. |
|
I'm reasonably happy with the idea of putting a donation link in the main navbar, but I'll make a more general comment about this over at #6605 It's a hard no from me on the iframe implementation though. It's really not a great user experience, it breaks bookmarks, leaves you unsure of which site you are adding payment information to, and so on. I would decline this PR on that basis alone. Perhaps you can consider having a page that introduces the topic of donations, and then the CTA takes you to support.openstreetmap.org - that might be better. It's worth considering that the target website is called "supporting" and not "donate", and it consists of multiple topics not just donations, when deciding what the link, url and content of the page here should be. |
|
Also links in the framed document that point to other sites like the wiki trigger CSP errors. I'd say it either links directly to the donation site, maybe opening in a new tab, or to a new page like @gravitystorm suggests that can then point them at the donation site. |
|
@Robot8A I forgot to say - your ruby code looks fine, I'm always happy to answer questions or help with the coding. Thanks for making the PR! |
5 participants
Would fix #6517
I went for an embed of https://supporting.openstreetmap.org/ on /donate. This way, the user can always return to the map or other parts of the website.
We can discuss whether this is the most appropriate way to do it or whether others would be better. Also, it's my first time programming with Ruby on Rails, so I hope I did everything in a way that makes sense :)