Conversation
**Why**: - Semantically, it's essentially the same thing. - Avoids needing to maintain our own component implementation. Instead, we can leverage the battle-tested USWDS solution. - Because we no longer implement our own solution, we can now eliminate jQuery as a dependency. - Better isolation of styles allows for some portability if we want to bring this into the design system.
| @@ -1,26 +1,4 @@ | |||
| import $ from 'jquery'; | |||
| {% if page.lang == 'en' %} | ||
| {% if locale == 'en' %} | ||
| {% assign locale_url = page.url %} | ||
| {% elsif locale == page.lang %} | ||
| {% assign locale_url = page.url %} | ||
| {% else %} | ||
| {% assign locale_url = page.url | prepend: locale | prepend: '/' %} | ||
| {% endif %} | ||
| {% else %} | ||
| {% if locale == 'en' and page.lang == 'es' %} | ||
| {% assign locale_url = page.url | remove_first: 'es/' %} | ||
| {% elsif locale == 'en' and page.lang == 'fr' %} | ||
| {% assign locale_url = page.url | remove_first: 'fr/' %} | ||
| {% elsif locale == page.lang %} | ||
| {% assign locale_url = page.url %} | ||
| {% else %} | ||
| {% assign locale_url = page.url | replace_first: page.lang, locale %} | ||
| {% endif %} | ||
| {% endif %} | ||
| <a href="{{ locale_url | prepend: site.baseurl }}" lang="{{ locale }}"> |
There was a problem hiding this comment.
I see this is old/unchanged, but it's making my brain melt, I feel like we could probably turn this into a custom plugin in ruby and simplify what it's doing (whatever that is....)
(Not for this PR necessarily)
There was a problem hiding this comment.
Need to do more testing, but I think we might be able to replace this with a one-liner:
| {% if page.lang == 'en' %} | |
| {% if locale == 'en' %} | |
| {% assign locale_url = page.url %} | |
| {% elsif locale == page.lang %} | |
| {% assign locale_url = page.url %} | |
| {% else %} | |
| {% assign locale_url = page.url | prepend: locale | prepend: '/' %} | |
| {% endif %} | |
| {% else %} | |
| {% if locale == 'en' and page.lang == 'es' %} | |
| {% assign locale_url = page.url | remove_first: 'es/' %} | |
| {% elsif locale == 'en' and page.lang == 'fr' %} | |
| {% assign locale_url = page.url | remove_first: 'fr/' %} | |
| {% elsif locale == page.lang %} | |
| {% assign locale_url = page.url %} | |
| {% else %} | |
| {% assign locale_url = page.url | replace_first: page.lang, locale %} | |
| {% endif %} | |
| {% endif %} | |
| <a href="{{ locale_url | prepend: site.baseurl }}" lang="{{ locale }}"> | |
| <a | |
| href="{{ page.url | remove_first: 'es/' | remove_first: 'fr/' | prepend: locale | remove_first: 'en/' | prepend: '/' | prepend: site.baseurl }}" | |
| lang="{{ locale }}" | |
| > |
There was a problem hiding this comment.
I think I misunderstood how remove_first works, so this might not work exactly as we want all the time, particularly when the path includes en/, fr/, es/ somewhere other than the start of the URL.
To your original suggestion, I think we could probably adapt our existing locale filters to support what we need here. I might do it separately.
I think I figured out why it wasn't being caught. The default viewport size for a Puppeteer page is quite small (800x600), and at that size the language dropdown isn't visible. We could increase the size, but then we might not catch issues which would only happen at smaller viewports 🤷 |
the puppeteer specs for accessibility are fairly fast, would it make sense to run them at two sizes? |
.usa-nav applies specific styles for links. Enforce a consistent padding and line-height on the language list items, and consistency to the language picker button via identical styling (padding, typeset).
Context: #674 (comment) **Why**: Due to media query styling, some issues are only present at one or the other viewport. Previously, the viewport was quite small so desktop-specific issues were not surfaced.
Yeah, I don't think it'd be much of an issue, and could help up discover similar issues like this one. I opened a separate pull request at #677, which should confirm (via failure) the issue being resolved here. |
Context: #674 (comment) **Why**: Due to media query styling, some issues are only present at one or the other viewport. Previously, the viewport was quite small so desktop-specific issues were not surfaced.
Context: #674 (comment) **Why**: Due to media query styling, some issues are only present at one or the other viewport. Previously, the viewport was quite small so desktop-specific issues were not surfaced.
2 participants
Why:
main.jsby 90%, from 98.8kb to 10.8kb)Visually, the element should appear mostly the same, but is not identical. Revisions should only move in the direction of being more faithful to the original design.