-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
WIP: Implement languages page #21882
Conversation
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.
Thanks so much for your PR, @wouterraateland! 🙏 🤗
This is looking great:
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.
This looks fantastic! Thank you so much for you work :)
In addition to the things Flo mentioned, I requested a couple of changes relating to layout and behavior. A lot of stuff hasn't been implemented yet from our side, so we'd like to comment out/remove those features from the language page for now until they can be implemented.
In addition, it seems there's some inconsistencies of what the "default language" means. Right now, the desired behavior is that clicking on any part of the <LanguageTableRow>
will change the UI to be that language. For example, clicking "Japanese" will redirect to /ja/languages/
.
Similarly, the language that's currently highlighted should be the current language. Not the "default" language, which currently is always English. Setting the "default language" here means "update the user's preferences in local storage so that gatsbyjs.org will redirect to gatsbyjs.org/ja (or something like that)"
Thanks for the reviews and clarification. Really useful feedback! Will work the requested changes tomorrow. |
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.
This looks really good! A couple of changes and we should be good to go as long as @fk approves.
I noticed you've commented out a lot of the features that aren't implemented yet. I know that you'd like to keep your work, but we're also not sure when and if we're going to implement the default language feature or the progress tracking. What I propose is:
- check out another branch based off of this one that has the additional features (default language selector and progress bar)
- remove the commented-out code from this PR relating to these features
- once we're ready to work on these features, I can assign them to you and you can start working off your updated branch.
Does that sound good?
</div> */} | ||
<MutedLink | ||
href={`https://github.com/gatsbyjs/gatsby-${lang.code}`} | ||
onClick={event => { |
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.
why is the stopPropagation
here needed?
Also, since this is an external link, it should have rel="noopener noreferrer"
and target="_blank"
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.
stopPropagation
is necessary because it is an <a/>
tag nested inside another <a/>
tag.
Could change the outer one so that doesn't render an <a />
tag, but just a div
with an onClick
listener. However, even then stopPropagation would be necessary.
Did add rel and target!
return ( | ||
<Row | ||
isCurrent={isCurrent} | ||
to={isDefault ? "/languages" : `/${lang.code}/languages`} |
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.
You can use getLocalizedLink
from utils/i18n
here to correctly account for the default language
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.
Couldn't find getLocalizedLink
, so used localizedPath
!
Sounds like a plan! I have removed the comments indeed. Let me know if something else needs to be changed. |
Just building this locally, basically 👍 👍— if I see something, I'll leave a suggestion (and sorry for not finding my way back here earlier! 🙏) |
Hi @wouterraateland, after discussion with @marcysutton, we've decided that the language page might need some redesign to make it accessible. In particular, nested links are inaccessible for keyboard users. While there are ways to get around this, another issue is that the design of the page doesn't match the implementation. The page looks like you're selecting from a set of options (like a radio button) but in fact you're just clicking a link. I'm talking to @fk right now to figure out a redesign but until then, could we table this PR? Thank you again for the work you've put in so far! Having an actual implementation helps us see the flaws with the current design and will help us towards a clean, accessible one. |
No worries @tesseralis . I agree, having the website accessible and understandable is important. Happy to help when the redesign is ready! |
Hey y'all! 👋 Nat and me got together to work on a clearer, simpler UI together and ended up following the reactjs.org pattern: Figma is here: https://www.figma.com/file/UcsUDRdCoJaQdfITkcKMnU/Language-toggle-UI?node-id=209%3A479 |
c96cac5
to
3188ed0
Compare
That redesign is looking clean! I have updated the code so that it should match the design. However, due to space constraints on my computer, I haven't been able to run the website and test... |
@wouterraateland unfortunately build time is an ongoing issue for us :( In the meantime, what I've been doing is running only on two languages. @pieh do you have any other advice or options? |
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.
This looks awesome! Thank you so much, @fk, for revisiting the design and @wouterraateland for sticking with us!
on the phone with Nat and she's good!
@fk would you mind taking one more look at this? It seems good to me but I don't know our styles as well as I'd like! |
👋 Hey Aisha! Just seeing this—of course, 👀now! |
@wouterraateland Hello again Wouter! Thanks for sticking with this, and us! 💜 This looks great so far! 🙏 I have two hopefully small-ish requests:
|
Replace fixed sizes with theme props on the language page. Co-Authored-By: Florian Kissling <21834+fk@users.noreply.github.com>
…iscelaneous fixes as requested
Hi guys, sorry for the long radio silence and @tesseralis thanks still for the advice. Today I've managed to get sufficient space on my PC and tried to build the website again. This time, the build succeeded, but on rendering, I got an error regarding invalid hooks usage. As the changes in this PR make no use of hooks, I'm not sure why this happens. @fk The changes you proposed where indeed minor and I think I have implemented them. Not being able to see the result makes it though to verify, but I have good hopes 🙏. Here is the stacktrace of the error for reference:
Could also open an issue with more details if necessary, let me know :) |
Hey Wouter! Welcome back, and don't worry about the "long silence"! 🤗 When updating my local version of your PR, I ran into this 🤷♂️ … … which I could resolve with a Anyway, latest updates are looking good 🙏 (just not sure why the "Gatsby" wordmark next to the monogram isn't showing in the header LOL 😉) — 👍 from me: |
Shoot hit the wrong button, sorry! |
Thanks @fk ! |
🎉 (and sorry for all the build pains!)
Yeah, also had me confused. I'll take another look now! |
` | ||
|
||
const allLanguages = [ | ||
{ code: "en", name: "English", localName: "English" }, |
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.
should better all languages are in the i18n.json so that there is only one place where the languages are placed (and/or tranlsated)?
</LocalName> | ||
<ContributionText> | ||
<ContributionLink | ||
href={`https://github.com/gatsbyjs/gatsby-${lang.code}`} |
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.
this link not work for en
because https://github.com/gatsbyjs/gatsby-en is not maintened
the server need also the current active language list in: |
Open questions
Description
Implements the languages page for gatsbyjs.org. Tested in both in light and dark mode.
Related Issues
Fixes #21750