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

Pixel perfect navbar toggler #21821

Merged
merged 1 commit into from
Jan 24, 2017
Merged

Conversation

tagliala
Copy link
Contributor

@tagliala tagliala commented Jan 23, 2017

Hi guys,

another opinionated PR, you may want to use another solution (such as use an actual 32px icon).

I've noticed that on non-retina displays, the navbar toggler icon is blurry.

The actual available size of the navbar toggler icon is 30px, but the
SVG uses a 32 unit grid. This commit uses a 30 unit grid and updates
icon accordingly.

The following images shows before and after this PR on non-retina macbook air
1st browser: Chrome 55
2nd browser: Safari 10.0.2
3rd browser: Firefox 50.1.0

image

The actual available size of the navbar toggler icon is 30px, but the
SVG uses a 32 unit grid. This commit uses a 30 unit grid and updates
icon accordingly.
@bardiharborow
Copy link
Member

Looks great. That's been annoying me for a while now, but I didn't realise it would be this easy to fix. <3

@pvdlg
Copy link
Contributor

pvdlg commented Jan 23, 2017

I don't know much about SVG...but the size of the icon will depends on the on the base font-size (which now come from the browser settings). So the size icon might change based on user settings.

The font-size for the toggler is 1.25rem and the height is 1.5rem. With a default base font-size of 16px that gives us 1.25 * 1.5 * 16 = 30px. But if the user set his default font-size to 14px then the icon would be 1.25 * 1.5 * 16 = 26.25px.

How would the SVG behave with a height of 26.25px ? Would it be blurry ?

@tagliala
Copy link
Contributor Author

@vanduynslagerp sorry but I can't answer, I only have a basic knowledge of SVG 🙌 .

It will be probably blurry, I've just tried to force 26.25px icon size

Result (before / after, chrome 55):
image

@pvdlg
Copy link
Contributor

pvdlg commented Jan 23, 2017

Either there is no differences or I don't have eyes good enough to see it.
It would make sense to me that the icon will be crisp only when it's rendered size match the SVG size, but maybe not...I really have no idea ¯_(ツ)_/¯

@mdo
Copy link
Member

mdo commented Jan 24, 2017

We could make it pixel based perhaps if that'd help, but it might be more difficult to customize in some capacities.

@mdo mdo merged commit b509dbe into twbs:v4-dev Jan 24, 2017
@mdo mdo mentioned this pull request Jan 24, 2017
@guylepage3
Copy link

I feel as though these are limitations to using rem vs px. We've chosen rem for a reason. Debates like "pixel perfect" display should have come up in the rem vs px debate. If they did not, and I don't recall them coming up, we should have that discussion now. No?

pvdlg added a commit to pvdlg/bootstrap that referenced this pull request Jan 27, 2017
* commit 'cfb25f6995160d1ba03da23c3a01446844f45fec':
  Add aria-label to docs search field
  Remove random cursor: default from pill nav (twbs#21835)
  Allow button toolbars to wrap (twbs#21826)
  fixes twbs#21567
  grunt (fixes twbs#21819)
  Add align self to navbar brand (twbs#21626)
  Rename order utilities to intended class names (twbs#21739)
  Pixel perfect navbar toggler (twbs#21821)
  Update _custom.scss imports in other builds (twbs#21825)
  Allow flex-based navs to wrap like they used to (twbs#21824)
  Remove `cursor: pointer;` (twbs#21812)
rudionrails added a commit to joblocal/bootstrap that referenced this pull request Jan 30, 2017
* bootstrap/v4-dev: (979 commits)
  Add aria-label to docs search field
  Remove random cursor: default from pill nav (twbs#21835)
  Allow button toolbars to wrap (twbs#21826)
  fixes twbs#21567
  grunt (fixes twbs#21819)
  Add align self to navbar brand (twbs#21626)
  Rename order utilities to intended class names (twbs#21739)
  Pixel perfect navbar toggler (twbs#21821)
  Update _custom.scss imports in other builds (twbs#21825)
  Allow flex-based navs to wrap like they used to (twbs#21824)
  Remove `cursor: pointer;` (twbs#21812)
  Fix body padding in Dashboard and Jumbotron examples.
  Move htmllint to npm script.
  Change to markdown (twbs#21815)
  Change header to use markdown (twbs#21809)
  Update grid layout docs (twbs#21806)
  grunt
  Drop Normalize, port relevant parts to Reboot (twbs#21741)
  grunt
  Fix container within navbar on smallest breakpoint (twbs#21722)
  ...
rudionrails added a commit to joblocal/bootstrap that referenced this pull request Jan 30, 2017
* bootstrap/v4-dev: (979 commits)
  Add aria-label to docs search field
  Remove random cursor: default from pill nav (twbs#21835)
  Allow button toolbars to wrap (twbs#21826)
  fixes twbs#21567
  grunt (fixes twbs#21819)
  Add align self to navbar brand (twbs#21626)
  Rename order utilities to intended class names (twbs#21739)
  Pixel perfect navbar toggler (twbs#21821)
  Update _custom.scss imports in other builds (twbs#21825)
  Allow flex-based navs to wrap like they used to (twbs#21824)
  Remove `cursor: pointer;` (twbs#21812)
  Fix body padding in Dashboard and Jumbotron examples.
  Move htmllint to npm script.
  Change to markdown (twbs#21815)
  Change header to use markdown (twbs#21809)
  Update grid layout docs (twbs#21806)
  grunt
  Drop Normalize, port relevant parts to Reboot (twbs#21741)
  grunt
  Fix container within navbar on smallest breakpoint (twbs#21722)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants