search

open-sauced / intro

Empowering Your Open Source Journey: From First Contribution to Project Leadership
https://opensauced.pizza/learn
Other
539 stars 110 forks source link

feat: add navbar to support multi language #42

Closed geoffreylgv closed 1 year ago

geoffreylgv commented 1 year ago

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)

Related Tickets & Documents

Closes #16

Mobile & Desktop Screenshots/Recordings

Added tests?

Added to documentation?

[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[bot] commented 1 year ago

Deploy Preview for sauced-intro ready!

Name Link
Latest commit 5148ad27cf47611542733dd002ff9199729ece03
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.

geoffreylgv commented 1 year ago

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

BekahHW commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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

BekahHW commented 1 year ago

lgtm!

antonio-pedro99 commented 1 year ago

Well done @geoffreylgv

CBID2 commented 1 year ago

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

geoffreylgv commented 1 year ago

Well done @geoffreylgv Woohoo! We did @geoffreylgv and @antonio-pedro99! Now onto PR https://github.com/open-sauced/intro/pull/38 @BekahHW!

Thank you all, we did it😃