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.

Sign up for GitHub

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

Jump to bottom

Stay on current page when switching lang, improve button alignment & make text configurable #740

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Stay on current page when switching lang, improve button alignment & make text configurable #740

wants to merge 2 commits into from

Conversation

kaistierl
Copy link

@kaistierl kaistierl commented Apr 13, 2020 •
edited
Loading

What this PR does / why we need it:

This mainly aims at improving the optics of the language switching buttons in multi-lang mode.

  • exclude the currently used language to yield space in the navbar
  • improve button optics and alignment in menu bar and collapsed menu
  • use long language name as button text to make it more user friendly and configurable
  • update documentation

The second commit changes the behaviour again:

  • We are now linking to translation of current page instead of home. Buttons are only shown if translations are present.

Special notes for your reviewer:

Release note:

Improved look and alignment of language switcher buttons, stay on current page when switching languages

exclude current lang in navbar, better alignment, use languageName
611a7ea 
* exclude the currently used language to yield space in the navbar
* improve button optics and alignment in menu bar and collapsed menu
* use long language name as button text to make it more user friendly
* update documentation
@ghost
Copy link

ghost commented Apr 13, 2020 •
edited by ghost
Loading

Congratulations 🎉. DeepCode analyzed your code in 1.236 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard

@kaistierl
Copy link
Author

kaistierl commented Apr 13, 2020 •
edited
Loading

I updated this PR since I again altered the implementation - now we will be staying on the current page when switching languages. Buttons are only shown for languages which have translations present for the current page.

@kaistierl kaistierl force-pushed the language_switcher_improvements branch from 194c5b3 to 28e7651 Compare April 13, 2020 22:38
@kaistierl kaistierl force-pushed the language_switcher_improvements branch from 28e7651 to 68b3e89 Compare April 14, 2020 08:06
@kaistierl
Copy link
Author

kaistierl commented Apr 14, 2020 •
edited
Loading

@stp-ip somehow the tests are failing but i see no relation to my change:

 Running:  modal.spec.js                                                                   (5 of 9)
  Modal
    1) should display modal when it's needed

  0 passing (808ms)
  1 failing

  1) Modal should display modal when it's needed:
     Uncaught TypeError: window.syna.showModal is not a function

What could be the cause for this? Is there something broken in the tests? I'd be glad if you could help...

@kaistierl kaistierl changed the title exclude current lang in navbar, better alignment, use languageName Stay on current page when switching lang, improve button alignment & make text configurable Apr 14, 2020
stp-ip
stp-ip reviewed Apr 14, 2020
View reviewed changes
Copy link
Member

@stp-ip stp-ip left a comment

Choose a reason for hiding this comment

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

Overall this seems like a good change. Thanks for taking the time to implement this.

Small action items to create issues for:

  • track migration from i18n to languageName
  • add multilingual "fragment" or docs showcasing the feature
  • multilingual /dev/ section + crypress tests

Not sure yet why the test failed. Thoughts @mpourismaiel ?

exampleSite/content/docs/multilingual/content.md
title = "Mon blogue"
weight = 2

[params]
...
```

The `languageName` parameter is used as text on the language switcher buttons
Copy link
Member

Choose a reason for hiding this comment

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

We are using i18n for this id being the language, but I think this is cleaner. We should open an issue tracking us to move from the i18n workaround to languageName globally in Syna.

@stp-ip stp-ip added this to the v0.17.0 milestone Apr 14, 2020
@stp-ip stp-ip modified the milestones: v0.17.0, v0.18.0 Apr 29, 2020
@stp-ip stp-ip modified the milestones: v0.17.1, v0.18.0 May 24, 2020
@mpourismaiel
Copy link
Member

@kaistierl Is there anything I can do to help finish this?

@stp-ip stp-ip modified the milestones: v0.17.3, v0.18.0 Jul 27, 2020
@stp-ip stp-ip modified the milestones: v0.17.4, v0.18.0 Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stp-ip stp-ip stp-ip left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement
Milestone
v0.18.0
Development

Successfully merging this pull request may close these issues.
3 participants
@kaistierl @mpourismaiel @stp-ip