infi-nl / the-infi-way

How we like to build software
https://way.infi.nl
Other
9 stars 8 forks source link

Language switching improvements #59

Closed h-five-e closed 1 year ago

h-five-e commented 1 year ago

Issue: https://github.com/infi-nl/the-infi-way/issues/38

netlify[bot] commented 1 year ago

Deploy Preview for the-infi-way ready!

Name Link
Latest commit b93b4a14e57de00176df1814b058718876e9e1d7
Latest deploy log https://app.netlify.com/sites/the-infi-way/deploys/6486dd3ed414e20008a1b777
Deploy Preview https://deploy-preview-59--the-infi-way.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

jeroenheijmans commented 1 year ago

Big improvement already! Some minor thoughts:

And one major question, thinking out loud:

LucaScorpion commented 1 year ago

Haven't had time to really dive into it yet, but looks good from what I can tell!

Are we good on the copyright and source of the icons?

@jeroenheijmans These are emoji, not images, so that should be fine :)

As for the discoverability of the language switcher, I agree somewhere at the top would be nicer (or at least more clear). But for now this is already a nice improvement imo.

Also to be safe: is there a reason it's still WIP, or would you be happy with it as-is @h-five-e ?

h-five-e commented 1 year ago

I'd like to stick with US English if a choice has to be made

I used what's already there, so just en. But your issue seems to be more with the flag. I am personally more in favour of UK English but as that is a personal preference I'll change the flag to the US one.

Also to be safe: is there a reason it's still WIP?

Yes:

Is it worth to do the language toggle at the top

Because I still have to fix this. Top right would be the intuitive placement for me as well. Kind of hesitant to mess around in someone else's css though 😅

Also I'm not really happy with how I did things, it feels kind of hacky. I think it would be better to have a separate JSON to define the languages instead of this "auto discovery".

h-five-e commented 1 year ago

This means that the default language is output twice, once to /index.html and once to en/index.html which imo is an okay trade off to incorporating server configuration and the small size of the project.

jeroenheijmans commented 1 year ago

@LucaScorpion will you have a peek at the final changes and merge if you're good? (Since you're currently the "assigned reviewer". If you want me to help get this one across that's also fine, let me know.)