[bug 1110543] Add noscript section #423
Conversation
|
I should probably add a trans tag to both file changes but let me know what you think. In the case of /feedback/templates/feedback/picker.html the could go right after the existing trans tag where as with /feedback/templates/feedback/generic_feedback.html it might be best to wrap {% trans %} around lines 47 and 50. |
| <!-- Noscript--> | ||
| <noscript> | ||
| <p>{{ _('JavaScript is required to leave feedback. Please enable JavaScript in your browser and refresh this page. Click <a href="https://support.mozilla.org/en-US/questions/967344">here</a> to learn how to enable JavasScript.') }}</p> | ||
| </noscript> |
There was a problem hiding this comment.
Instead of putting the url in the string to be translated, we use a {% trans %} block and make the url a variable. That way localizers don't have to worry about mistyping it plus we can change it later without having to re-translate strings.
Also, you want to nix the en-US part of the url. That'll get added when the user goes to that url, but it'll add the locale they want to see. If you hard-code the en-US part, then all users will go to en-US.
There was a problem hiding this comment.
Having said all that, I really think if someone has disabled JavaScript, then they know how to re-enable it. Plus SUMO doesn't work well if the user has JavaScript disabled. I think for now I'm inclined to just leave out that instructive sentence.
There was a problem hiding this comment.
Also, we probably want to figure out a way to make this note more prominent. Maybe add the "error" class.
|
I made the changes you suggested. |
| @@ -44,6 +44,12 @@ | |||
| <div class="card" id="intro" data-title="{{ _('{product} feedback')|fe(product=product.display_name) }}"> | |||
| <section> | |||
| <div class="ask"> | |||
| <noscript class="error"> | |||
There was a problem hiding this comment.
The noscript shouldn't be of class error. You should have the <p> outside the {% trans %} tags and that should have class error.
Generally, we don't want to put HTML in the strings that localizers translate because it means we can't change it without changing the string requiring them to re-translate and they sometimes do funny things with HTML which causes problems.
|
That's for letting me know about the {% trans %} stuff - I wasn't sure if |
| @@ -32,6 +32,13 @@ | |||
| <p class="no-products">{{ _('No products available.') }}</p> | |||
| {% else %} | |||
| <div class="ask"> | |||
| <p class="error"> | |||
There was a problem hiding this comment.
I think this is missing the <noscript> start tag.
|
Thanks for catching that Will, I just fixed that and pushed a new commit. |
|
Looks great! Squash these four commits into one and then we'll merge it. |
aokoye
force-pushed
the
1110543_nonscript
branch
from
fa9f69c
to
1fc67a3
Compare
|
I think I successfully squashed the 4 commits into 1 |
|
Awesome! |
[bug 1110543] Add noscript section
|
1fc67a3 [bug 1110543] Add noscript section |
Adding the section to both the generic_feedback.html and picker.html templates allows the message to stay displayed when the user picks a product without JavaScript being enabled.