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

Dropdown Menu for Language #5087

Merged
merged 9 commits into from
Mar 1, 2022

Conversation

SaravgiYash
Copy link
Contributor

Added a dropdown menu for the Language feature.

Screenshot

Desktop:
image
Mobile:
image

Stakeholders

@mekarpeles

@jdlrobson
Copy link
Collaborator

While I haven't pulled down the code and looked at this, I really like the direction here. As we modernize the UI, I am trying to keep the client as light as possible. My intention is that we will lean on JavaScript only for workflows such as the lists feature and editor. These will be completely Vue based.

Navigation is something that should work without JavaScript, for a variety of reasons, but mainly because HTML is more future proof because of its backwards compatible nature. Right now, we're currently grappling with upgrading jQuery UI experiences that if taken away would break key components of the site, and I'm keen to avoid that in essential parts of the user interface like this one. I'd also like us to move away from jQuery and show/hide patterns so we don't get stuck into jQuery in the same way.

I am in the process of putting together a document explaining a roadmap for the frontend and a guiding philosophy but the approach outlined here definitely sits in well with that! Thanks for trying out different ideas. This one seems to fit in nicely! I urge you to continue it. Experimenting with language and then applying it to all over menus once proved seems like a great approach here.

Copy link
Collaborator

@jdlrobson jdlrobson left a comment

Choose a reason for hiding this comment

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

Looks great but doesn't appear to be functional yet.

openlibrary/templates/lib/nav_foot.html Outdated Show resolved Hide resolved
@jdlrobson
Copy link
Collaborator

I completely agree with you but even localhost:8080?lang=fr doesn't show any visual changes.

I know, we'd need a backend change to support that, but I see that as a necessary part of this solution.

@SaravgiYash
Copy link
Contributor Author

@jdlrobson What should be my next step? I will add the query string parameter. Anything else that I need to do?
Apart from that, if the dropdown is fine, I will start working on the header dropdown PR.

@jdlrobson
Copy link
Collaborator

@mekarpeles can you advise how i18n should work without javascript, cookies disabled or for local development?

@mekarpeles mekarpeles added the Priority: 3 Issues that we can consider at our leisure. [managed] label Jun 28, 2021
@SaravgiYash
Copy link
Contributor Author

SaravgiYash commented Jul 21, 2021

If we merge #5367 then we can remove all the changes related to the mobile view from this PR which will remove most of the duplicate code.

@mekarpeles
Copy link
Member

mekarpeles commented Feb 3, 2022

@Yashs911 I think we may be able to move forward with this one since #5367 is merged

It looks like some conflicts need to be resolved

@SaravgiYash
Copy link
Contributor Author

@Yashs911 I think we may be able to move forward with this one since #5367 is merged

It looks like some conflicts need to be resolved

@mekarpeles I have fixed the merge conflict and made the necessary fixes.

I have also removed all the changes from the footer section for this PR as now we have IA Bar on Mobile, So do let me know how we want to proceed with the Footer section language list (completely removing it or merging PR 5367) because as you mentioned #5367 is not merged yet

@SaravgiYash
Copy link
Contributor Author

Jest: "global" coverage threshold for statements (19%) not met: 18.81%
Jest: "global" coverage threshold for lines (19%) not met: 18.86%
Jest: "global" coverage threshold for functions (15%) not met: 14.81%

If someone can guide me on how to fix the above error and pre-commit error

@mekarpeles mekarpeles added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Feb 7, 2022
@mekarpeles mekarpeles merged commit 5d94aa9 into internetarchive:master Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] Priority: 3 Issues that we can consider at our leisure. [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants