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: style fixes for the TOC #4748

Closed
wants to merge 2 commits into from

Conversation

silverwind
Copy link
Contributor

Followup for #4621, which unfortunately hadn't gotten enough enough browser testing besides Chrome/Firefox on OS X. I tested on Chrome, Firefox, Safari and IE11 in all modes down to IE7 on Windows and OS X.

  • Hide the scrollbar on the TOC on all browsers. It was never the intention for it to be visible with the scroll indication in place. A wrapper element was necessary to achieve the desired effect.
  • Fixed the scroll indication gradient on Safari, which was caused by the wrong from-color, which now matches the to-color.
  • Fixed a issue in old IE where the TOC didn't render on the correct position through setting left: 0 and top: 0 on it.

cc: @nodejs/documentation

@silverwind silverwind added the doc Issues and PRs related to the documentations. label Jan 18, 2016
- Hide the scrollbar on the TOC on all browsers. It was never the
  intention for it to be visible with the scroll indication in place.
  A wrapper element was necessary to achieve the desired effect.
- Fixed the scroll indication gradient on Safari, which was caused by
  the wrong from-color, which now matches the to-color.
- Fixed a issue in old IE where the TOC didn't render on the correct
  position through setting `left: 0` and `top: 0` on it.
@jasnell
Copy link
Member

jasnell commented Jan 19, 2016

LGTM

@rvagg
Copy link
Member

rvagg commented Jan 19, 2016

/cc @nodejs/website just to make sure there's a bit of sync on styling

@silverwind
Copy link
Contributor Author

Forgot to mention, but the prime motivation for removing the scrollbar was that the fade-out gradient was rendering above the scrollbar on platforms other than OS X platforms (which has overlay scrollbars in all browsers).

I tried various hacks to remove the scrollbars and the most simple and compatible solution was to hide the scrollbar in a overflow: hidden container that is 16px wider than the child containing the content.

@xcatliu
Copy link

xcatliu commented Jan 19, 2016

@silverwind
A discuss about whether we should support IE8: nodejs/nodejs.org#467 (comment)

@fabiosantoscode
Copy link
Contributor

Never assume scroll bars are 16px wide. On my elementaryos system every
scrollbar is thinner. Chrome uses its own thing but firefox uses the system
one. Also, it's trivial to measure them using JavaScript!

On 02:51, Tue, 19 Jan 2016 Xcat Liu [email protected] wrote:

@silverwind https://github.com/silverwind
A discuss about whether we should support IE8: nodejs/nodejs.org#467
(comment)
nodejs/nodejs.org#467 (comment)


Reply to this email directly or view it on GitHub
#4748 (comment).

@silverwind
Copy link
Contributor Author

@fabiosantoscode You're right, it's a bad assumption. Calculating scrollbar width seems a rather error-prone business to me, it does require JS, right?

@fabiosantoscode
Copy link
Contributor

I would certainly avoid doing it, but it's still a lot better than assuming it's only 16px ;)

@silverwind
Copy link
Contributor Author

Oh, by the way: I don't think we actually need to account for scrollbars smaller than 16px because there's a lot of empty spacing in the TOC. Smaller scrollbars shouldn't matter as no content is lost off-screens, it's only the bigger ones that would cause issues.

@fabiosantoscode
Copy link
Contributor

scrollBarWidth = elm.offsetWidth - elm.clientWidth, provided there are no borders or padding.

silverwind added a commit that referenced this pull request Jan 23, 2016
- Hide the scrollbar on the TOC on all browsers. It was never the
  intention for it to be visible with the scroll indication in place.
  A wrapper element with 20px padding was added to accommodate for
  hopefully all scrollbar widths as well as to avoid overflowing
  content.
- Fixed the scroll indication gradient on Safari, which was caused by
  the wrong from-color, which now matches the to-color.
- Fixed a issue in old IE where the TOC didn't render on the correct
  position through setting `left: 0` and `top: 0` on it.

PR-URL: #4748
Reviewed-By: James M Snell <[email protected]>
@silverwind
Copy link
Contributor Author

Landed in 55607a0.

I think the point brought up by @fabiosantoscode is, while valid, not a concern because there's no content being hidden off-screen (the TOC entries are not wide enough), so it doesn't matter if the scrollbar is less wide.

To accomodate for possibly wider system scrollbars, I changed the value to 20px padding, which helps to ensure that no content is lost outside the visible area.

@silverwind silverwind closed this Jan 23, 2016
rvagg pushed a commit that referenced this pull request Jan 25, 2016
- Hide the scrollbar on the TOC on all browsers. It was never the
  intention for it to be visible with the scroll indication in place.
  A wrapper element with 20px padding was added to accommodate for
  hopefully all scrollbar widths as well as to avoid overflowing
  content.
- Fixed the scroll indication gradient on Safari, which was caused by
  the wrong from-color, which now matches the to-color.
- Fixed a issue in old IE where the TOC didn't render on the correct
  position through setting `left: 0` and `top: 0` on it.

PR-URL: #4748
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
- Hide the scrollbar on the TOC on all browsers. It was never the
  intention for it to be visible with the scroll indication in place.
  A wrapper element with 20px padding was added to accommodate for
  hopefully all scrollbar widths as well as to avoid overflowing
  content.
- Fixed the scroll indication gradient on Safari, which was caused by
  the wrong from-color, which now matches the to-color.
- Fixed a issue in old IE where the TOC didn't render on the correct
  position through setting `left: 0` and `top: 0` on it.

PR-URL: #4748
Reviewed-By: James M Snell <[email protected]>
@silverwind
Copy link
Contributor Author

This still needs backporting. Which tag is preferred for cases like these by the way?

@MylesBorins
Copy link
Contributor

@silverwind it had the correct tag. It is in v4.x-staging. Will be in the next release

@MylesBorins
Copy link
Contributor

For context

lts-watch-v4.x not yet in staging
land-on-v4.x in staging, not yet released
lts-landed-on-v4.x released

@silverwind
Copy link
Contributor Author

Ah, got it! Am I free to land this on v4.x myself?

@MylesBorins
Copy link
Contributor

@silverwind we do not land on v4.x unless there is a release going on. Landing on staging is as good as landing on v4.x

This makes it very easy for us to do security releases without cherry picking.

TLDR; v4.x is only touched when there is a release

@silverwind
Copy link
Contributor Author

Ah, alright, thanks for the explanation!

MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
- Hide the scrollbar on the TOC on all browsers. It was never the
  intention for it to be visible with the scroll indication in place.
  A wrapper element with 20px padding was added to accommodate for
  hopefully all scrollbar widths as well as to avoid overflowing
  content.
- Fixed the scroll indication gradient on Safari, which was caused by
  the wrong from-color, which now matches the to-color.
- Fixed a issue in old IE where the TOC didn't render on the correct
  position through setting `left: 0` and `top: 0` on it.

PR-URL: #4748
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
- Hide the scrollbar on the TOC on all browsers. It was never the
  intention for it to be visible with the scroll indication in place.
  A wrapper element with 20px padding was added to accommodate for
  hopefully all scrollbar widths as well as to avoid overflowing
  content.
- Fixed the scroll indication gradient on Safari, which was caused by
  the wrong from-color, which now matches the to-color.
- Fixed a issue in old IE where the TOC didn't render on the correct
  position through setting `left: 0` and `top: 0` on it.

PR-URL: nodejs#4748
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 11, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 13, 2016
- Hide the scrollbar on the TOC on all browsers. It was never the
  intention for it to be visible with the scroll indication in place.
  A wrapper element with 20px padding was added to accommodate for
  hopefully all scrollbar widths as well as to avoid overflowing
  content.
- Fixed the scroll indication gradient on Safari, which was caused by
  the wrong from-color, which now matches the to-color.
- Fixed a issue in old IE where the TOC didn't render on the correct
  position through setting `left: 0` and `top: 0` on it.

PR-URL: nodejs#4748
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
- Hide the scrollbar on the TOC on all browsers. It was never the
  intention for it to be visible with the scroll indication in place.
  A wrapper element with 20px padding was added to accommodate for
  hopefully all scrollbar widths as well as to avoid overflowing
  content.
- Fixed the scroll indication gradient on Safari, which was caused by
  the wrong from-color, which now matches the to-color.
- Fixed a issue in old IE where the TOC didn't render on the correct
  position through setting `left: 0` and `top: 0` on it.

PR-URL: nodejs#4748
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
- Hide the scrollbar on the TOC on all browsers. It was never the
  intention for it to be visible with the scroll indication in place.
  A wrapper element with 20px padding was added to accommodate for
  hopefully all scrollbar widths as well as to avoid overflowing
  content.
- Fixed the scroll indication gradient on Safari, which was caused by
  the wrong from-color, which now matches the to-color.
- Fixed a issue in old IE where the TOC didn't render on the correct
  position through setting `left: 0` and `top: 0` on it.

PR-URL: nodejs#4748
Reviewed-By: James M Snell <[email protected]>
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.

6 participants