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

Add Pride Styling to the Masterbar #17669

Merged
merged 10 commits into from Sep 1, 2017

Conversation

Projects
None yet
8 participants
@pento
Member

pento commented Aug 31, 2017

Some years ago, we rainbow-ed the Masterbar in support of SCOTUS' marriage equality ruling.

To show our support for regional marriage equality initiatives, this PR allows the rainbow Masterbar to be enabled by browser language, giving us a way to target visitors based on a region they're probably associated with, not just where they're located.

For example, by adding en-AU as a prideLanguage, we catch most people in Australia, as well as Australians around the world.

Props @MichaelArestad for the first Masterbar rainbow design, which I've shamelessly copied.

Reviews

There are a few reviews needed for this:

  • Design - Does the CSS need any tweaking?
  • i18n - Is the i18n-utils module a good place for getAcceptedLanguagesFromHeader()?
  • Code - This is adding the class deep in the guts of the SSR code. Is it likely to cause any SSR problems?

pento added some commits Aug 31, 2017

Masterbar: Add pride styling
Based on the original work by @MichaelArestad.
@matticbot

This comment has been minimized.

Show comment
Hide comment

matticbot commented Aug 31, 2017

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Aug 31, 2017

Member

Desktop screenshots:

Logged in view
Logged out view

Member

pento commented Aug 31, 2017

Desktop screenshots:

Logged in view
Logged out view

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Aug 31, 2017

Member

Mobile screenshots:

Logged in view
Logged out view

Member

pento commented Aug 31, 2017

Mobile screenshots:

Logged in view
Logged out view

@alisterscott

This comment has been minimized.

Show comment
Hide comment
@alisterscott

alisterscott Aug 31, 2017

Contributor

Is there some way to indicate this is for a particular cause, and/or link to https://australianmarriageequality.org (I know this will be difficult on mobile as the width is narrow)

Contributor

alisterscott commented Aug 31, 2017

Is there some way to indicate this is for a particular cause, and/or link to https://australianmarriageequality.org (I know this will be difficult on mobile as the width is narrow)

@alisterscott

This comment has been minimized.

Show comment
Hide comment
@alisterscott

alisterscott Aug 31, 2017

Contributor

Even some optional hover text for desktop layout so someone hovering over the header knows what it is about

Contributor

alisterscott commented Aug 31, 2017

Even some optional hover text for desktop layout so someone hovering over the header knows what it is about

Show outdated Hide outdated server/pages/index.js
@Tug

This comment has been minimized.

Show comment
Hide comment
@Tug

Tug Aug 31, 2017

Contributor

Code 👍
I did an experiment and tried to move the condition to the context. That would have allowed us to keep the customizations on the client. Sadly I failed cause I did not realized the Masterbar is rendered in the Layout (so outside the context which is related to a page.js route). That would have required quite a lot of changes.

Contributor

Tug commented Aug 31, 2017

Code 👍
I did an experiment and tried to move the condition to the context. That would have allowed us to keep the customizations on the client. Sadly I failed cause I did not realized the Masterbar is rendered in the Layout (so outside the context which is related to a page.js route). That would have required quite a lot of changes.

Show outdated Hide outdated server/pages/index.js
Show outdated Hide outdated server/pages/index.js
Show outdated Hide outdated client/lib/i18n-utils/utils.js
@markryall

Code looks great to me.

If design been approved we should 🚢

@raoulwegat raoulwegat self-requested a review Sep 1, 2017

@raoulwegat

This comment has been minimized.

Show comment
Hide comment
@raoulwegat

raoulwegat Sep 1, 2017

Contributor

Design LGTM 🏳️‍🌈
Time to 🚢

Contributor

raoulwegat commented Sep 1, 2017

Design LGTM 🏳️‍🌈
Time to 🚢

@raoulwegat raoulwegat removed their request for review Sep 1, 2017

@pento pento merged commit 3a6a3dd into master Sep 1, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
ci/i18n 0 new strings. ¡Ándale!
Details

@pento pento deleted the add/pride-styling branch Sep 1, 2017

@MichaelArestad

This comment has been minimized.

Show comment
Hide comment
@MichaelArestad

MichaelArestad Sep 1, 2017

Contributor

CC @ebinnion, my coconspirator.

Contributor

MichaelArestad commented Sep 1, 2017

CC @ebinnion, my coconspirator.

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