Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added language selector #2531

Merged
merged 17 commits into from
Aug 9, 2023
Merged

feat: added language selector #2531

merged 17 commits into from
Aug 9, 2023

Conversation

werdnanoslen
Copy link
Contributor

@werdnanoslen werdnanoslen commented Jul 26, 2023

Summary

  1. Adds Language Selector component
  2. Adds a pass-through prop to Menu for showing the right kind of NavList (using subnav as default if not given, as is currently hard-coded)
  3. Adds language type to NavList

Related Issues or PRs

Resolves #2516

How To Test

Open the stories for LanguageSelector

Screenshots (optional)

Two languages just toggles:
two

More than two language shows a dropdown:
link

@werdnanoslen
Copy link
Contributor Author

This uses links per USWDS' implementation, but I think it's also likely that people will want to implement in-page with buttons using onClick (similar to their guidance here, resulting in our other PR). This is also how my current project expects to use it, so I'll look into a button option as a follow-up commit here or future PR.

- added both buttons and links, depending on given click action
- added missing click action to 2-lang button
- added custom class story
@werdnanoslen
Copy link
Contributor Author

This uses links per USWDS' implementation, but I think it's also likely that people will want to implement in-page with buttons using onClick (similar to their guidance here, resulting in our other PR). This is also how my current project expects to use it, so I'll look into a button option as a follow-up commit here or future PR.

Latest commit adds this, so ready for review (pending styles ⚠️ )!

@werdnanoslen
Copy link
Contributor Author

This uses links per USWDS' implementation, but I think it's also likely that people will want to implement in-page with buttons using onClick (similar to their guidance here, resulting in our other PR). This is also how my current project expects to use it, so I'll look into a button option as a follow-up commit here or future PR.

Latest commit adds this, so ready for review (pending styles ⚠️ )!

Fixed styling now that 3.3 styles are merged, and filed an issue to let us remove that later if merged. This is fully ready for review now

@werdnanoslen
Copy link
Contributor Author

Hm, not sure I'm interpreting the build rule correctly, but given my @extend is the simplest way I know of to reference code from USWDS and not copy-paste it into our code, could this error be overridden?

Copy link
Contributor

@jcbcapps jcbcapps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Just left a couple of minor questions. Didn't feel like they were big enough to request changes.

src/components/LanguageSelector/LanguageSelector.tsx Outdated Show resolved Hide resolved
src/styles/index.scss Outdated Show resolved Hide resolved
@werdnanoslen werdnanoslen requested a review from jcbcapps August 1, 2023 16:27
}: LanguageSelectorButtonProps &
JSX.IntrinsicElements['button']): React.ReactElement => {
const classes = classnames('usa-button', 'usa-language__link', className)
const labelNotEn = labelAttr && <span lang={labelAttr}>{label}</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you removed the curLangNotEn variable in LanguageSelector and are no longer using the labelAttr prop from there, do we still need this labelNotEn variable? Looking at the test coverage, the <span> here is not checked, and it just made me wonder if it is still needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still keep it on the button so that the button contents get the same lang attr treatment. I just updated it to handle that better if you want to see those changes


type LanguageSelectorButtonProps = {
label: string
labelAttr?: string
Copy link
Contributor

@jcbcapps jcbcapps Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need the labelAttr prop? It seems that in order for this prop to be utilized, LanguageSelectorButton would need to be used outside of the LanguageSelector component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's still useful for the 2 language variant, where the language's text should be tagged. I've updated it a bit to reflect that

@shkeating
Copy link
Contributor

so what I have gathered so far is the lint tool is mad at how you used @extend because it can cause specificity issues. I personally don't feel this particular instance carries much of that risk, especially as a temporary solution parked in the index file. I'm seeing if there is a way to push the link styles from uswds dynamically that doesn't use @extend.

I'm pretty sure those styles are stored in a mixin already which we can call with @include for a similarly elegant solution that does not make us have to override lint rules

@shkeating
Copy link
Contributor

so what I have gathered so far is the lint tool is mad at how you used @extend because it can cause specificity issues. I personally don't feel this particular instance carries much of that risk, especially as a temporary solution parked in the index file. I'm seeing if there is a way to push the link styles from uswds dynamically that doesn't use @extend.

I'm pretty sure those styles are stored in a mixin already which we can call with @include for a similarly elegant solution that does not make us have to override lint rules

im now seeing that the styles for this particular link instance are pretty custom. maybe our quickest path forward is just grabbing the scss and pasting it where @extend would have output it. not as sleek but it is a temp solution. im trying that now to see if it works

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@use 'uswds-core' as *; would need to be added up top for the suggestion to work

Copy link
Contributor

@haworku haworku Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@werdnanoslen did you include this change too when you tested the utilities? You need to have uswds-core in scope for utilities to work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, but i kept getting build errors

// This just allows buttons as well as links in the LanguageSelector
// It can be removed when https://github.com/uswds/uswds/issues/5409 is fixed
.usa-language__submenu .usa-language__submenu-item button {
@extend a;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@extend a;
color: color('white');
display: block;
line-height: line-height($theme-navigation-font-family, 3);
padding: 0;
padding: units(1);
text-decoration: none;
width: 100%; /* this is something that was actually missing when we were doing the extend because button widths hve different default widths than block element links */
&:focus {
outline-offset: units('neg-05');
}
&:hover {
color: color('white');
text-decoration: underline;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this just grips the scss right out of the package, replicating the @extend output.

only advantage of this over using @extend is it stops us from overriding lint rules, and it makes sure the focus indicator behaves the same with with width assignment I added, but you could also add that using the @extend.

I would defer to the app eng folks on how they feel about overriding lint rules, but if you are looking to get this going this is how I'd get around it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! committed that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, we don't seem to be using utilities anywhere, and I couldn't get it to work, so I've just copied the compiled styles over here for now.

)

if (langs.length > 2) {
const [isOpen, setIsOpen] = useState(false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought i'd jump the gun a bit and try this in IRS project. Getting the following eslint error there:

React Hook "useState" is called conditionally. React Hooks must be called in the exact same order in every component render.eslint[react-hooks/rules-of-hooks](https://reactjs.org/docs/hooks-rules.html)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

)
}
const [langIndex, setLangIndex] = useState(false)
const curLang = langs[Number(langIndex)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting the same eslint error about this instance ofuseState as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

shkeating
shkeating previously approved these changes Aug 9, 2023
@werdnanoslen
Copy link
Contributor Author

Happo all looks as intended, except for the ios-mobile ones:
Screenshot 2023-08-09 at 2 22 28 PM

However, I can't reproduce this myself, so maybe it's a Happo issue?
Screenshot 2023-08-09 at 2 21 09 PM

I'm going to self-approve Happo unless someone is able to reproduce that issue.

Copy link
Contributor

@shkeating shkeating left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also cannot reproduce the safari issue

@werdnanoslen werdnanoslen merged commit 60bf797 into main Aug 9, 2023
@werdnanoslen werdnanoslen deleted the an-language-2516 branch August 9, 2023 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Component: Language Selector
5 participants