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

doc: fix inability to hover version-picker #34447

Closed
wants to merge 4 commits into from
Closed

doc: fix inability to hover version-picker #34447

wants to merge 4 commits into from

Conversation

DmitryScaletta
Copy link

@DmitryScaletta DmitryScaletta commented Jul 20, 2020

See this #34447 (comment)

Checklist

0.25rem = height of `li` - offset for `ol.version-picker` = 1rem - 1.25rem
`left` and `right` is -1 because of the border
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jul 20, 2020
Copy link
Member

@Trott Trott 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

Copy link

@willin willin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@juanarbol juanarbol 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!

@Trott Trott 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

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

@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

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

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;

Lato

Lucida Sans Unicode

Verdana

Tahoma

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

After fix:

Lato

Lucida Sans Unicode

Verdana

Tahoma

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

@DmitryScaletta DmitryScaletta 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

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

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

@silverwind
Copy link
Contributor

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.

@jasnell jasnell 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

#34768 looks like an alternative solution.

BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
PR-URL: #34768
Fixes: #34767
Fixes: #34447
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
PR-URL: #34768
Fixes: #34767
Fixes: #34447
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #34768
Fixes: #34767
Fixes: #34447
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #34768
Fixes: #34767
Fixes: #34447
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet