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

burger size is not responsive to navbar height #1534

Closed
jimblue opened this issue Dec 16, 2017 · 22 comments · May be fixed by #1535
Closed

burger size is not responsive to navbar height #1534

jimblue opened this issue Dec 16, 2017 · 22 comments · May be fixed by #1535
Labels

Comments

@jimblue
Copy link

jimblue commented Dec 16, 2017

Overview of the problem
This is about the Bulma CSS framework
I'm using Bulma version [0.6.1]

Description
Burger size is fixed. It's a problem when changing the navbar height for higher value as the burger will look too small.

Expected behavior
Changing the navbar height should have an influence on the burger size.

@raygervais
Copy link

@jimblue, @jgthms
I agree entirely with the expected behavior here and would love to contribute toward a fix for this. I use this throughout quite a few projects and would love to have the burger size being responsive as one would expect.

@raygervais
Copy link

@jimblue, I interpreted the overall burger size as the button since it was an upgrade that I wanted as well. Was this correct, or did you mean the spacing in between the burger elements?

@jimblue
Copy link
Author

jimblue commented Dec 17, 2017

@raygervais
I mean the icon size inside the button.
The button actually does resize responsively but if it's bigger than the default size the burger icon will look too small. (the 3 lines and the cross)

It's because the burger mixin define the width of burger element's in px.
Look the code here:

width: 16px

@jimblue
Copy link
Author

jimblue commented Dec 17, 2017

In my opinion there is two solutions:

  • modify the mixin to make it responsive with rem value
  • passing an argument to the mixin that change elements size

@raygervais
Copy link

I see, agree that the 3 spans / lines need to be responsive as well.
I suppose my interpretation derived from my experience of reducing the height of the navigation bar, and that causing issue with the hamburger menu.

I believe the first solution you propose would be the correct fix.

If that is the case then I'd revert back my change since it is truly is already responsive and somehow I missed that. Then will look at the optimal algorithm for sizing. We could easily use the dimension itself which is passed in by the function signature:

=hamburger($dimensions)

@jimblue
Copy link
Author

jimblue commented Dec 17, 2017

Cool, so you have an idea how to modify the mixin to do so?
Seem a bit complicated as there is not only the with to modify.
That could brake the animation.

@raygervais
Copy link

Animation would have to be updated to accommodate the dynamic width of the spans, but this process wouldn't break the animation I imagine.

I imagine a dynamic calculation such as calc($dimension - ($dimension / 2.5)) could do the trick for widths, since this also takes into account the margins. Note that 2.5 is just a hypothetical calculation, the value itself would be anywhere between 2.01 and n if my math is correct.

Animation width would take this into account in a similar fashion.

@jimblue
Copy link
Author

jimblue commented Dec 19, 2017

Ok let's do it!! How can I help?

@raygervais
Copy link

raygervais commented Dec 19, 2017

@jimblue,
Can you confirm this is the expected behavior?

Default Height:
image

Double Height, Still requires an update to the algorithm for the spacing of the hamburger 'layers'
image

Half Width Burger Element:
image
Double Width Burger Element:
image

As you can see, the idea that I'm proposing (still up for debate) is to make the width of the lines 40%, which was based on calculations I had done for the padding and margins which scale with the width.
The height of the 'layers' would follow suit if you think this is the correct / desired functionality. Apologies if I've missed the mark on what you were expecting. 😃

This addresses the width, the actual height of the burger and it's layers would take on a similar logic.

@jimblue
Copy link
Author

jimblue commented Dec 19, 2017

Hey @raygervais !

I see using percentage is kind of working but it breaks the square ratio of the button which is not good.

Changing the navbar height already change the burger button to keep a square size.

That mean the 3 lines should use responsive value relative to the button size to not break the ratio.

What could be nice is also to space the 3 lines a bit more when the button get bigger.

In the end I think it would be probably better to define in px 3 sizes:

  • is-normal (the actual default size)
  • is-medium
  • is- big

That way user could decide...

What do you think?

@raygervais
Copy link

Adding size modifier classes makes quite a bit of sense for a framework such as Bulma, it follows similar practice to other elements. The responsive classes may fix the issue which you and I mentioned. Currently, the square doesn't change on 0.6.1 when you change the height of the navbar, I am using the latest codebase. If it is supposed to, can you show even just via an image?

Regarding these responsive classes, they me be the perfect class to utilize the percentage calculations, this way we can have a 16:9, 4:4, etc. I'll explore more of this when I have a chance today / tomorrow.

@jimblue
Copy link
Author

jimblue commented Dec 19, 2017

I confirm that the square does change:

default 3.25rem
3 25rem

5rem
5rem

7rem
7rem

@raygervais
Copy link

Interesting, I don't see the responsive change in my repository, perhaps I'm behind on a critical patch. Will revert any changes I've made, update the branch and apply the concepts we've discussed here.

Do you believe these are appropriate class names?

  • is-bigger-burger => Forces the widths to be 80% let's say of the overall size
  • is-default-burger => Standard, widths appropriate to 16px in REM
  • is-smaller-burger => Forces the widths to be 20% let's say of the overall size

@jimblue
Copy link
Author

jimblue commented Dec 19, 2017

I think it would make more sens to use the following class on .burger element:

  • is-small
  • is-medium
  • is-large

It's enough right?

Agree for the rest 😄

@raygervais
Copy link

Agreed 👍

@raygervais
Copy link

@jimblue,
I disregarded is-medium since that is the default hamburger menu calculations, unless you can think of a good reason as to why, or I figured omitting is-large or is-small results in the defaults. is-small looks to be too close to the bottom, but I figured I'd get your opinion on that first. Once we are done here, I'll update the PR with the spec.

Here are what the classes look like based on our agreed upon sizing conventions, with the included is-active to ensure the animation isn't broken:

is-large

image

is-active

image

is-small

image

is-active

image

@jimblue
Copy link
Author

jimblue commented Dec 21, 2017

Hey!!

Great job @raygervais ! 😄

The small doesn't look too close to the bottom IMO.

It's look like Bulma spirit is respected here, I think you can go for the PR !

@raygervais
Copy link

Thanks @jimblue, appreciate the help with figuring out the details here!
I'll update the PR, and then get @jgthms to review for any issues / concerns.
👍

@jimblue
Copy link
Author

jimblue commented Dec 21, 2017

You're welcome, thanks to you for the PR

@devism
Copy link

devism commented Jan 19, 2018

This is desperately needed!

@raygervais
Copy link

@devism, see the PR :)

@stale
Copy link

stale bot commented Jan 21, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 21, 2019
@stale stale bot closed this as completed Jan 28, 2019
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 a pull request may close this issue.

3 participants