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.

Sign up for GitHub

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

Jump to bottom

WIP: Dropdown menu for language #4862

Closed
wants to merge 7 commits into from
Closed

WIP: Dropdown menu for language #4862

wants to merge 7 commits into from

Conversation

SaravgiYash
Copy link
Contributor

@SaravgiYash SaravgiYash commented Mar 18, 2021 •
edited
Loading

Technical

Added a dropdown menu for the Language feature.

Screenshot

Footer_Trim.mp4

Stakeholders

@mekarpeles
Copy link
Member

mekarpeles commented Mar 18, 2021 •
edited
Loading

The linter is showing the following errors which may be blocking js from building?:

static/css/components/footer.less
 140:39  ✖  Unexpected leading zero                                                                         number-leading-zero      
 144:1   ✖  Expected selector ".dropdown-content a" to come before selector "#footer-icons a"               no-descending-specificity
 144:1   ✖  Expected selector ".dropdown-content a" to come before selector "#footer-links ul li a"         no-descending-specificity
 151:1   ✖  Expected selector ".dropdown-content a:hover" to come before selector "#footer-icons a"         no-descending-specificity
 151:1   ✖  Expected selector ".dropdown-content a:hover" to come before selector "#footer-links ul li a"   no-descending-specificity

@mekarpeles
Copy link
Member

mekarpeles commented Mar 18, 2021 •
edited
Loading

I think we probably want the style / experience to be pretty similar to http://scholar.archive.org in the top header
scholar archive org_
scholar archive org_ (2)

jdlrobson
jdlrobson requested changes Mar 19, 2021
View reviewed changes
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.

Please reuse the CSS classes for the dropdowns in the menu (and markup including checkbox). Building these in the same way will cut down on css and js we use on the site and the component functionality is identical which would be a performance win!

jdlrobson
jdlrobson reviewed Mar 19, 2021
View reviewed changes
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.

Another thing to consider.. since this is a JavaScript only feature, should we build it in Javascript rather than inside the template?

jdlrobson
jdlrobson reviewed Mar 19, 2021
View reviewed changes
static/css/components/footer.less Outdated
@@ -123,3 +123,45 @@ div#version-details {
border-radius: 3px;
padding: 8px 10px;
}

.dropbtn {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feature doesn't work without javascript from what I can see and footer.less is render blocking. Styles should be loaded inside all.js.

Copy link
Contributor Author

@SaravgiYash SaravgiYash Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, but from what I have noticed that Browse, More, and Account components do not support keyboard navigation so is there a way to implement it in the current structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If not I guess we should make this dropdown keeping keyboard navigation in mind.
Later we can implement the same for other dropdown menus in the header.

@jdlrobson jdlrobson marked this pull request as draft March 20, 2021 17:55
@SaravgiYash
Copy link
Contributor Author

SaravgiYash commented Mar 23, 2021 •
edited
Loading

@jdlrobson, can you please guide me on how to load styles inside all.js? Do I need to load an external CSS file or write the styles in jquery?

@SaravgiYash
Copy link
Contributor Author

SaravgiYash commented Mar 23, 2021 •
edited
Loading

I think we probably want the style / experience to be pretty similar to http://scholar.archive.org in the top header

@mekarpeles If I am not wrong do you want this feature in the top header of openlibrary.org or only the styles should be similar?
Is this implementation correct?? (checkbox dropdown)

concept.mp4

@jdlrobson
Copy link
Collaborator

Put the import inside static/css/js-all.less

@SaravgiYash
Copy link
Contributor Author

SaravgiYash commented Mar 24, 2021 •
edited
Loading

@jdlrobson I made a separate less file and imported it inside js-all.less but still it's not working

@jdlrobson
Copy link
Collaborator

Have you run npm run watch ?

@SaravgiYash
Copy link
Contributor Author

Have you run npm run watch ?

No, only make js and make css

@jdlrobson
Copy link
Collaborator

make js is what you need to do but, bunning npm run watch will allow you to debug better as you don't have to run make js each time. For CSS loaded in JavaScript make js will do nothing.

I suggest adding some console.log statements in openlibrary/plugins/openlibrary/js/index.js and checking the line import '../../../../static/css/js-all.less'; is working by adding something obvious like body {background: rebeccapurple !important;} to js-all.less and the new file you are importing to debug what's going wrong for you.

If you can't work it out, feel free to push up what you have and we can work out why it's not working.

@SaravgiYash
Copy link
Contributor Author

SaravgiYash commented Mar 27, 2021 •
edited
Loading

@jdlrobson Now I am confused 😕. I went to the master branch and did make css, make js, and npm run watch. Restarted the web container and when I click the Language option nothing changed(visually). So I checked the cookies and could see the HTTP_LANG
image
Even on the work branch (design-footer-lang), I am able to see the cookie. So I guess nothing is wrong with this PR.
Could you please check if you can see any visual changes on the localhost without applying any styles?

EDIT: I tried adding ?href=fr but nothing changed visually so I guess we are good to go.

jdlrobson
jdlrobson reviewed Mar 27, 2021
View reviewed changes
static/css/components/footer-locale.less Outdated
@@ -0,0 +1,55 @@
.language-component__checkbox {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename to language.less - footer-locale is an instance of this component and reusable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jdlrobson strange how does renaming changes the bundlesize.
FAIL static/build/all.js: 128KB > maxSize 128KB (gzip)

Copy link
Collaborator

Choose a reason for hiding this comment

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

#footer-locale is shorter than .language-component__checkbox and gzips better. Bundlesize is checking the size of the CSS file generated from this source code.

Copy link
Contributor Author

@SaravgiYash SaravgiYash Mar 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We have id="footer-locale-menu" on the main div.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The code here is fine. I was responding to your comment about it being stranged that it impacted bundlesize and explaining why that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should we fix this. Increase bundlesize?

@SaravgiYash
Copy link
Contributor Author

#5087

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdlrobson jdlrobson jdlrobson requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SaravgiYash @mekarpeles @jdlrobson