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

Fix language switcher responsiveness #11977

Conversation

AlexVelezLl
Copy link
Member

@AlexVelezLl AlexVelezLl commented Mar 12, 2024

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:

image

After:

image

Reviewer guidance

  1. Test with the setup wizard
  2. Go to the step of choosing a default language for Kolibri
  3. Play with the window width checking that the amount of languages displayed are the optimal to keep it in one line.

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added DEV: frontend DEV: tools Internal tooling for development SIZE: medium labels Mar 12, 2024
Copy link
Contributor

github-actions bot commented Mar 12, 2024

@AlexVelezLl AlexVelezLl marked this pull request as ready for review April 3, 2024 15:24
@AlexVelezLl AlexVelezLl force-pushed the fix-language-switcher-responsiveness branch from 84f7986 to c5c0ca5 Compare April 3, 2024 18:48
@AlexVelezLl AlexVelezLl requested a review from pcenov April 3, 2024 18:48
@github-actions github-actions bot added the APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) label Apr 3, 2024
@AlexVelezLl AlexVelezLl removed OS: Linux DEV: tools Internal tooling for development labels Apr 3, 2024
Copy link
Member

@pcenov pcenov left a 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.

@AlexVelezLl
Copy link
Member Author

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.

image

@github-actions github-actions bot added APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: tools Internal tooling for development labels Apr 17, 2024
@AlexVelezLl
Copy link
Member Author

Hi @pcenov, this should be solved now! I would really appreciate it if you could take another look at it 👐.

@pcenov
Copy link
Member

pcenov commented Apr 18, 2024

Hi @AlexVelezLl see the following video:

2024-04-18_11-59-33.mp4

In 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.

@AlexVelezLl
Copy link
Member Author

Thanks @pcenov! I'll give a look at that case.

@AlexVelezLl
Copy link
Member Author

Hi @pcenov. This should be fixed now! Could you take a look please? =)

@pcenov
Copy link
Member

pcenov commented Apr 23, 2024

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

@AlexVelezLl
Copy link
Member Author

AlexVelezLl commented Apr 24, 2024

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.

@pcenov
Copy link
Member

pcenov commented Apr 24, 2024

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.

@marcellamaki
Copy link
Member

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?

Screenshot 2024-04-24 at 8 43 46 AM

But as I suggest that, I realize this is probably not a default behavior in KListWithOverflow though (although I am less familiar than you are, certainly).

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.

@AlexVelezLl
Copy link
Member Author

AlexVelezLl commented Apr 24, 2024

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.

Copy link
Member

@marcellamaki marcellamaki left a 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.

@rtibbles rtibbles merged commit 504c2e3 into learningequality:develop May 3, 2024
30 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: frontend DEV: tools Internal tooling for development SIZE: medium TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup wizard: Responsive language list
4 participants