Skip to content

doc: fix inability to hover version-picker#34447

Closed
DmitryScaletta wants to merge 4 commits intonodejs:masterfrom
DmitryScaletta:patch-1
Closed

doc: fix inability to hover version-picker#34447
DmitryScaletta wants to merge 4 commits intonodejs:masterfrom
DmitryScaletta:patch-1

Conversation

@DmitryScaletta
Copy link

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

@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

willin
willin approved these changes Jul 22, 2020
View reviewed changes
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

juanarbol
juanarbol reviewed Jul 22, 2020
View reviewed changes
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 •
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

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

@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

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.

@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

richardlau commented Aug 14, 2020

#34768 looks like an alternative solution.

@aduh95 aduh95 mentioned this pull request Aug 14, 2020
Closed
@tniessen tniessen mentioned this pull request Aug 17, 2020
Closed
2 tasks
@addaleax addaleax closed this in 6f0b87d Aug 19, 2020
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
@Vap0r1ze @BethGriggs
doc: remove space above version picker
1de6ba3 
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
@Vap0r1ze @BethGriggs
doc: remove space above version picker
5214de7 
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
@Vap0r1ze @addaleax
doc: remove space above version picker
022b2aa 
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
@Vap0r1ze @addaleax
doc: remove space above version picker
6d9e6f6 
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

Reviewers

@juanarbol juanarbol juanarbol left review comments

@Trott Trott Trott approved these changes

+6 more reviewers

@osk2 osk2 osk2 approved these changes

@willin willin willin approved these changes

@hiroppy hiroppy hiroppy approved these changes

@aymen94 aymen94 aymen94 approved these changes

@alexandrtovmach alexandrtovmach alexandrtovmach approved these changes

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

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.