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

feat: add navbar to support multi language #42

Merged
merged 4 commits into from
Sep 8, 2023
Merged

feat: add navbar to support multi language #42

merged 4 commits into from
Sep 8, 2023

Conversation

geoffreylgv
Copy link
Contributor

Description

This PR is a feature adding a sidebar for the multi-language version of the intro course.

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

Closes #16

Mobile & Desktop Screenshots/Recordings

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

Added to documentation?

  • 📜 README.md
  • 📓 docs.opensauced.pizza
  • 🍕 dev.to/opensauced
  • 📕 storybook
  • 🙅 no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

@netlify
Copy link

netlify bot commented Sep 1, 2023

Deploy Preview for sauced-intro ready!

Name Link
🔨 Latest commit 5148ad2
🔍 Latest deploy log https://app.netlify.com/sites/sauced-intro/deploys/64fb79e5218de7000862ab15
😎 Deploy Preview https://deploy-preview-42--sauced-intro.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 configuration.

Copy link
Contributor

@antonio-pedro99 antonio-pedro99 left a comment

Choose a reason for hiding this comment

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

Nice job @geoffreylgv.
Apart from my specific comments, here is one of the reasons to not including French related translation in this PR.

  • This PR introduces the basic structure to support multiple language.
  • Once merged, to add a new idiom translation, I would just add the language in the _layouts/navbar.md and create a new folder under translations with the language short form, like pt-br for Brazilian Portuguese.

This is to avoid conflicts with #38 and #35.
And also, you can not add semi translated docs here. If this PR gets merged now, users will see that there's a French version, but they won't see because it is not available yet. :)


IMHO, the only change we need in this PR is:

* Languages
    * [:us: English](/)

The rest you can include in #38.

_layouts/navbar.md Outdated Show resolved Hide resolved
translations/_layouts/navbar.md Outdated Show resolved Hide resolved
translations/_layouts/sidebar.md Outdated Show resolved Hide resolved
translations/fr/README.md Outdated Show resolved Hide resolved
@geoffreylgv
Copy link
Contributor Author

Nice job @geoffreylgv. Apart from my specific comments, here is one of the reasons to not including French related translation in this PR.

  • This PR introduces the basic structure to support multiple language.
  • Once merged, to add a new idiom translation, I would just add the language in the _layouts/navbar.md and create a new folder under translations with the language short form, like pt-br for Brazilian Portuguese.

This is to avoid conflicts with #38 and #35. And also, you can not add semi translated docs here. If this PR gets merged now, users will see that there's a French version, but they won't see because it is not available yet. :)

IMHO, the only change we need in this PR is:

* Languages
    * [:us: English](/)

The rest you can include in #38.

Hello @antonio-pedro99
I've removed french related contents to have a single temple for the navbar
This means in the translations/country-code/, we should have _layouts/sidebar.md and _layouts/navbar.md to have the navbar and sidebar in the specific language we are translating

Copy link
Contributor

@antonio-pedro99 antonio-pedro99 left a comment

Choose a reason for hiding this comment

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

LGTM
Left a comment.

index.html Show resolved Hide resolved
@BekahHW
Copy link
Member

BekahHW commented Sep 5, 2023

When I'm in dark mode, the text for the language gets lighter. I think this is a docsify thing, but let me know if there's a different setting we need to add.

Also, do you know why the dropdown with the language disappears if you don't comment out <!-- <link rel="stylesheet" href="./styles/custom.css" /> -->

@geoffreylgv
Copy link
Contributor Author

When I'm in dark mode, the text for the language gets lighter. I think this is a docsify thing, but let me know if there's a different setting we need to add.

Hi @BekahHW, for the dark/light mode on language dropdown, it's a docsify thing; let dig on settings that can help to have it also changeable.

Also, do you know why the dropdown with the language disappears if you don't comment out <!-- <link rel="stylesheet" href="./styles/custom.css" /> -->

And for the CSS commented, hmm there is a way actually to have the centered navbar behavior

@BekahHW
Copy link
Member

BekahHW commented Sep 7, 2023

@geoffreylgv for the commented out portion, does that impact css elsewhere? I haven't tested it out, but before merging this in, I want to make sure that it's not impacting things I haven't seen

@geoffreylgv
Copy link
Contributor Author

@geoffreylgv for the commented out portion, does that impact css elsewhere? I haven't tested it out, but before merging this in, I want to make sure that it's not impacting things I haven't seen

No it doesn't affect, @BekahHW
In the meantime I'll check the CSS and propose a minor change (maybe write one line or edit twoo in the custom style) if we should keep the CSS line not commented.

@geoffreylgv
Copy link
Contributor Author

@geoffreylgv for the commented out portion, does that impact css elsewhere? I haven't tested it out, but before merging this in, I want to make sure that it's not impacting things I haven't seen

Hey @CBID2 @BekahHW @antonio-pedro99
I will kindly ask to check this behavior on your end

By editing the position property from unset to relative
position: unset !important;
position: relative !important;

Final app-nav style

.app-nav {
	display: flex;
	align-items: center;
	justify-content: center;
	position: relative !important;
	margin-top: 16px !important;
	margin-right: unset !important;
}

Basically, this means to keep the <link rel="stylesheet" href="./styles/custom.css" /> uncommented in the html and to edit the custom.css file
The unset property tells to ignore any inherited position property on the app-nav who is static by default

@CBID2
Copy link
Contributor

CBID2 commented Sep 8, 2023

@geoffreylgv for the commented out portion, does that impact css elsewhere? I haven't tested it out, but before merging this in, I want to make sure that it's not impacting things I haven't seen

Hey @CBID2 @BekahHW @antonio-pedro99 I will kindly ask to check this behavior on your end

By editing the position property from unset to relative position: unset !important; position: relative !important;

Final app-nav style

.app-nav {
	display: flex;
	align-items: center;
	justify-content: center;
	position: relative !important;
	margin-top: 16px !important;
	margin-right: unset !important;
}

Basically, this means to keep the <link rel="stylesheet" href="./styles/custom.css" /> uncommented in the html and to edit the custom.css file The unset property tells to ignore any inherited position property on the app-nav who is static by default

Hey @geoffreylgv. I can't test it locally unless you grant me permission to push code to your PR. Invite me to your repo

@geoffreylgv
Copy link
Contributor Author

@geoffreylgv for the commented out portion, does that impact css elsewhere? I haven't tested it out, but before merging this in, I want to make sure that it's not impacting things I haven't seen

Hey @CBID2 @BekahHW @antonio-pedro99 I will kindly ask to check this behavior on your end
By editing the position property from unset to relative position: unset !important; position: relative !important;
Final app-nav style

.app-nav {
	display: flex;
	align-items: center;
	justify-content: center;
	position: relative !important;
	margin-top: 16px !important;
	margin-right: unset !important;
}

Basically, this means to keep the <link rel="stylesheet" href="./styles/custom.css" /> uncommented in the html and to edit the custom.css file The unset property tells to ignore any inherited position property on the app-nav who is static by default

Hey @geoffreylgv. I can't test it locally unless you grant me permission to push code to your PR. Invite me to your repo

Ok @CBID2 done! Check your mail

@CBID2
Copy link
Contributor

CBID2 commented Sep 8, 2023

@geoffreylgv for the commented out portion, does that impact css elsewhere? I haven't tested it out, but before merging this in, I want to make sure that it's not impacting things I haven't seen

Hey @CBID2 @BekahHW @antonio-pedro99 I will kindly ask to check this behavior on your end
By editing the position property from unset to relative position: unset !important; position: relative !important;
Final app-nav style

.app-nav {
	display: flex;
	align-items: center;
	justify-content: center;
	position: relative !important;
	margin-top: 16px !important;
	margin-right: unset !important;
}

Basically, this means to keep the <link rel="stylesheet" href="./styles/custom.css" /> uncommented in the html and to edit the custom.css file The unset property tells to ignore any inherited position property on the app-nav who is static by default

Hey @geoffreylgv. I can't test it locally unless you grant me permission to push code to your PR. Invite me to your repo

Ok @CBID2 done! Check your mail

Thanks @geoffreylgv. I pushed the code. The text does come out light in dark mode, but once I hover over the language name, it darkens a bit. As far as other parts @BekahHW, they're ok.

testing-navbar.mov

@BekahHW
Copy link
Member

BekahHW commented Sep 8, 2023

lgtm!

@BekahHW BekahHW merged commit 38f26f8 into open-sauced:main Sep 8, 2023
4 checks passed
@CBID2 CBID2 deleted the feat-adding-navbar branch September 8, 2023 21:08
@antonio-pedro99
Copy link
Contributor

Well done @geoffreylgv

@CBID2
Copy link
Contributor

CBID2 commented Sep 8, 2023

Woohoo! We did @geoffreylgv and @antonio-pedro99! Now onto PR #38 @BekahHW!

@geoffreylgv
Copy link
Contributor Author

Well done @geoffreylgv
Woohoo! We did @geoffreylgv and @antonio-pedro99! Now onto PR #38 @BekahHW!

Thank you all, we did it😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Translate the course to other language
4 participants