Skip to content

doc: fix inability to hover version-picker#34447

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
Closed
DmitryScaletta wants to merge 4 commits into from
Closed

doc: fix inability to hover version-picker #34447

DmitryScaletta wants to merge 4 commits into from

Conversation

@DmitryScaletta
Copy link

@DmitryScalettaDmitryScaletta commented Jul 20, 2020•
edited
Loading

See this #34447 (comment)

Checklist

@DmitryScaletta
doc: fix inability to hover version-picker
75f5571
0.25rem = height of `li` - offset for `ol.version-picker` = 1rem - 1.25rem `left` and `right` is -1 because of the border
@nodejs-github-botnodejs-github-bot added the doc Issues and PRs related to the documentations. label Jul 20, 2020
Trott
Trott approved these changes Jul 22, 2020
View reviewed changes
Copy link
Member

@TrottTrott left a comment

Choose a reason for hiding this comment

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

/ping @nodejs/website

willin
willin approved these changes Jul 22, 2020
View reviewed changes
Copy link

@willinwillin left a comment

Choose a reason for hiding this comment

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

LGTM

juanarbol
juanarbol reviewed Jul 22, 2020
View reviewed changes
Copy link
Member

@juanarboljuanarbol left a comment

Choose a reason for hiding this comment

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

Thank you!

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 22, 2020
@lpinca
Copy link
Member

lpinca commented Jul 22, 2020•
edited
Loading

No objections but I can't reproduce the issue on Firefox on Windows or Chrome/Safari on macOS.

@DmitryScaletta
Copy link
Author

DmitryScaletta commented Jul 23, 2020•
edited
Loading

@lpinca
Steps to reproduce this bug:

  1. Go to any page in docs (for example https://nodejs.org/api/fs.html)
  2. Hover link "View another version"
  3. Slowly move the cursor down

You can zoom the page up to 200% to see this bug for sure.

In some cases it looks fine.
It depends which font uses to render and which font is installed in the system from the font-family property.

@lpinca
Copy link
Member

lpinca commented Jul 23, 2020

I don't have this issue. There is no space between the elements as shown in the issue description. See the screenshot attached below

screenshot

@DmitryScalettaDmitryScaletta marked this pull request as draft July 23, 2020 13:48
@DmitryScaletta
Copy link
Author

DmitryScaletta commented Jul 23, 2020

Hmm. Looks like there is a problem with fonts.
Different fonts renders differently and the current layout relies on one of them:

font-family: Lato,"Lucida Grande","Lucida Sans Unicode","Lucida Sans", Verdana, Tahoma, sans-serif;
Details

Lato

Lucida Sans Unicode

Verdana

Tahoma

To fix that we need to set fixed height for the menu items.

After fix:

Details

Lato

Lucida Sans Unicode

Verdana

Tahoma

There was also bug in the mobile version witch is also fixed:

Details

@DmitryScalettaDmitryScaletta marked this pull request as ready for review July 23, 2020 16:28
@Trott
Copy link
Member

Trott commented Jul 23, 2020

Not opposing, but I too cannot replicate unless the width/text-size is such that the menu wraps. Chrome macOS

@richardlau
Copy link
Member

richardlau commented Jul 23, 2020

I can reproduce on Firefox on Windows but only if my mouse cursor is to the right of the dropdown arrow (and left of the right edge of the drop down box).

@DmitryScaletta
Copy link
Author

DmitryScaletta commented Jul 30, 2020

So I updated the code. See details here #34447 (comment)
Looks like it needs re-review.

@silverwind
Copy link
Contributor

silverwind commented Jul 31, 2020

This seems like a huge hack overall. I'd suggest a rewrite to a <select> box. Besides the dropdown content, It can be made to look similar with a bit of CSS.

@jasnelljasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 7, 2020
@jasnell
Copy link
Member

jasnell commented Aug 7, 2020

Given the comment here and @silverwind's feedback, I've removed the author ready label.

@richardlau
Copy link
Member

richardlau commented Aug 14, 2020

#34768 looks like an alternative solution.

@aduh95aduh95 mentioned this pull request Aug 14, 2020
Closed
@tniessentniessen mentioned this pull request Aug 17, 2020
Closed
2 tasks
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
@Vap0r1ze@BethGriggs
doc: remove space above version picker
1de6ba3
PR-URL: #34768Fixes: #34767Fixes: #34447 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Harshitha K P <[email protected]> Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
@Vap0r1ze@BethGriggs
doc: remove space above version picker
5214de7
PR-URL: #34768Fixes: #34767Fixes: #34447 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Harshitha K P <[email protected]> Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@Vap0r1ze@addaleax
doc: remove space above version picker
022b2aa
PR-URL: #34768Fixes: #34767Fixes: #34447 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Harshitha K P <[email protected]> Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@Vap0r1ze@addaleax
doc: remove space above version picker
6d9e6f6
PR-URL: #34768Fixes: #34767Fixes: #34447 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Harshitha K P <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juanarboljuanarboljuanarbol left review comments

@TrottTrottTrott approved these changes

+6 more reviewers

@osk2osk2osk2 approved these changes

@willinwillinwillin approved these changes

@hiroppyhiroppyhiroppy approved these changes

@aymen94aymen94aymen94 approved these changes

@alexandrtovmachalexandrtovmachalexandrtovmach approved these changes

@3imed-jaberi3imed-jaberi3imed-jaberi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

docIssues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

14 participants

@DmitryScaletta@lpinca@Trott@richardlau@silverwind@jasnell@osk2@willin@hiroppy@aymen94@juanarbol@alexandrtovmach@3imed-jaberi@nodejs-github-bot