FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: fix inability to hover version-picker by DmitryScaletta · Pull Request #34447 · nodejs/node · GitHub

/ node Public

doc: fix inability to hover version-picker - #34447

Closed
DmitryScaletta wants to merge 4 commits into
nodejs:masterfrom
DmitryScaletta:patch-1
ConversationCommits (4)ChecksFiles changed
Closed

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

Conversation

DmitryScaletta commented Jul 20, 2020 •
edited
Loading

Copy link
Copy Markdown

See this #34447 (comment)

Checklist

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 added the doc Issues and PRs related to the documentations. label Jul 20, 2020
Trott approved these changes Jul 22, 2020
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

/ping @nodejs/website

willin approved these changes Jul 22, 2020
View reviewed changes

willin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

juanarbol reviewed Jul 22, 2020
View reviewed changes

juanarbol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you!

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 22, 2020

lpinca commented Jul 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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

DmitryScaletta commented Jul 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Author

@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 commented Jul 23, 2020

Copy link
Copy Markdown
Member

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

DmitryScaletta marked this pull request as draft July 23, 2020 13:48

DmitryScaletta commented Jul 23, 2020

Copy link
Copy Markdown
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;
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 marked this pull request as ready for review July 23, 2020 16:28

Trott commented Jul 23, 2020

Copy link
Copy Markdown
Member

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

richardlau commented Jul 23, 2020

Copy link
Copy Markdown
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 commented Jul 30, 2020

Copy link
Copy Markdown
Author

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

silverwind commented Jul 31, 2020

Copy link
Copy Markdown
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 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 7, 2020

jasnell commented Aug 7, 2020

Copy link
Copy Markdown
Member

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

richardlau commented Aug 14, 2020

Copy link
Copy Markdown
Member

#34768 looks like an alternative solution.

aduh95 mentioned this pull request Aug 14, 2020
Closed
tniessen mentioned this pull request Aug 17, 2020
Closed
2 tasks
addaleax closed this in 6f0b87d Aug 19, 2020
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
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
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
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

juanarbol juanarbol left review comments
Trott Trott approved these changes
+6 more reviewers
osk2 osk2 approved these changes
willin willin approved these changes
hiroppy hiroppy approved these changes
aymen94 aymen94 approved these changes
alexandrtovmach alexandrtovmach approved these changes
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.

Back | FazBrowse Home | New Git URL