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

v4 hamburger icon fix #20260

Closed
wants to merge 3 commits into from
Closed

v4 hamburger icon fix #20260

wants to merge 3 commits into from

Conversation

patrickhlauke
Copy link
Member

closes #20234

@@ -115,6 +115,12 @@
@include hover-focus {
text-decoration: none;
}

&::before {
content: "\2261"; // see http://unicode.johnholtripley.co.uk/2261/

Choose a reason for hiding this comment

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

Properties should be ordered font-weight, content

@patrickhlauke
Copy link
Member Author

comparison before/after screenshot (admittedly the \2261 glyph is a bit anemic - hence the bump in font-weight - and not quite as wide as I'd like...)

hamburglar

@patrickhlauke
Copy link
Member Author

@mdo @cvrebert thoughts?


&::before {
font-weight: 500;
content: "\2261"; // see http://unicode.johnholtripley.co.uk/2261/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe comment it as IDENTICAL TO (U+2261) similar to

content: "\2039";// SINGLE LEFT-POINTING ANGLE QUOTATION MARK (U+2039)
?

@cvrebert
Copy link
Collaborator

cvrebert commented Jul 8, 2016

I'd also suggest adding a comment explaining why we can't use TRIGRAM FOR HEAVEN (U+2630).
Documenting these sorts of design decisions / historical context is best-practice.

content: "\2261"; // IDENTICAL TO (U+2261)
// wider overall support (see http://unicode.johnholtripley.co.uk/2261/)
// compared to previously used TRIGRAM FOR HEAVEN (U+2630)
// using generated content, rather than adding this in markup,
Copy link
Collaborator

@cvrebert cvrebert Jul 8, 2016

Choose a reason for hiding this comment

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

Erm, where did we establish that using generated content made any difference WRT emoji?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, i got confused

@cvrebert
Copy link
Collaborator

cvrebert commented Jul 8, 2016

LGTM technically speaking. I can't speak as to the visual design; @mdo ought to weigh in.

@mdo
Copy link
Member

mdo commented Jul 8, 2016

Aesthetically that looks a bit off right now. I'd be curious to see an exploration that use an SVG background image like we do with our validation icons for comparison.

@patrickhlauke
Copy link
Member Author

patrickhlauke commented Jul 9, 2016

rough sketch of how an SVG approach may look like https://jsfiddle.net/patrick_h_lauke/nu5ejdy3/5/

@cvrebert
Copy link
Collaborator

cvrebert commented Jul 9, 2016

A survey of implementation options: https://css-tricks.com/three-line-menu-navicon/

@patrickhlauke
Copy link
Member Author

patrickhlauke commented Jul 10, 2016

tricks using borders, shadows etc just feel...extremely hacky and old-school to me. my personal preference would be either an SVG or unicode approach (I guess SVG would give us more flexibility in design of course, so not adverse to changing this to match my rough fiddle https://jsfiddle.net/patrick_h_lauke/nu5ejdy3/5/ ... @mdo thoughts?)

@XhmikosR
Copy link
Member

I agree with @mdo that visually the new one doesn't look so good.

The SVG one does look a lot better IMO.

@patrickhlauke
Copy link
Member Author

@mdo @cvrebert @XhmikosR should i update this PR to take the approach i tried in my jsfiddle instead then? I'm tending towards that one too myself

@XhmikosR
Copy link
Member

Personally I think yes. But wait for the others to confirm too.

@cvrebert
Copy link
Collaborator

Sounds like we're onboard with SVG. I'd say go for it Patrick.

@patrickhlauke
Copy link
Member Author

closing in favor of successor #20329 which uses SVG

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants