-
Notifications
You must be signed in to change notification settings - Fork 738
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
Fix language switcher responsiveness #11977
Fix language switcher responsiveness #11977
Conversation
Build Artifacts
|
84f7986
to
c5c0ca5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @AlexVelezLl! No issues observed while manually testing.
Just found that this PR was creating an overflow on the login page, and had some KListWithOverflow styles interfering with the language switcher, so we could wait for learningequality/kolibri-design-system#612 to be released before merging this pr. |
Hi @pcenov, this should be solved now! I would really appreciate it if you could take another look at it 👐. |
Hi @AlexVelezLl see the following video: 2024-04-18_11-59-33.mp4In smaller resolutions we can get into a salutation where we only have the 'More languages' label left on its own or even overlapping with the last remaining language. I think in such cases we should always see the globe icon, one language and the 'More languages' label on the next line. |
Thanks @pcenov! I'll give a look at that case. |
Hi @pcenov. This should be fixed now! Could you take a look please? =) |
Hi @AlexVelezLl - I think it's definitely improved but I am still able to get to a state where only the 'More languages' button is displayed (when having selected languages with longer names such as Portuguese (Mozambique). This is valid for the setup wizard, the home page and create an account: languages.mp4 |
Hmm, that's because we don't have enough space to render both the language and the button @pcenov. What would be the correct behavior in that case? We could try to add a validation and show "change language" instead of "more languages" if the current language isn't shown. |
Hi @AlexVelezLl it's best to discuss this with @marcellamaki - I'm just pointing out that previously we were always showing the current language plus the 'More languages' link. |
Hi @pcenov and @AlexVelezLl - thank you for the collaboration so far here 🎉 Alex, I am wondering how complicated it would be to have the current language "stack" above the "more languages" button? So at this point, these become block rather than inline? But as I suggest that, I realize this is probably not a default behavior in If that's not the default behavior, I do think at some point it might be worth considering how to extend the list for cases like this (especially with the mobile responsiveness perspective in mind), but we don't necessarily want to make that change just yet. Hmmm.... I'm going to think about this some more. I welcome any thoughts :) Misha may also have some ideas. |
Hi @marcellamaki! Yes, I was also wondering if this should come into KListWithOverflow. I don't think this should be the default behavior because I think in cases like the KTabs, we wouldn't want this stacked behavior. I tried to achieve this with a boolean prop and avoided hiding the last element if the prop was true, but it was becoming much more complex to handle inside the KListWithOverflow. For now I propose handling it inside the LanguageSwitcherList with this more simpler solution that is just show the selected language inside the more button if all list items are overflowed. Other Ideas that come to my mind can be to have an #empty slot, and we just show the empty slot instead of the more button if it is provided and all items are overflowed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think going with a simpler approach for now, rather than adding a lot of complexity to KListWithOverflow
is a better idea. If we continue to discover other ways where that component should be extended or modified, we can think about making changes, but for now, that seems unnecessary and potentially like it could lead to unwanted behaviors in other places.
Thank you, Alex. This looks good.
Summary
Fix the responsiveness of the LanguageSwitcherList using the new KListWithOverflow that takes into account the space available to render the number of languages that fit on a single line.
References
Closes #11923
Screenshots
Before:
After:
Reviewer guidance
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)