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
Homepage: Display bhm link instead of video for en-US #39027
Conversation
type: "celebrate_bhm", | ||
} | ||
|
||
actions.push(I18n.locale == :"en-US" ? bhm_action : code_video_action) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(very minor) Is locale a symbol or a string? (In most places, I see us just compare to en-US
but don't know which is more correct. Either way, not too important.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Symbol, I tried as a string, it didn't work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also curious whether there's a standard of how we determine locale? I see elsewhere in that same file we look at request.locale, do you happen to know the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not - I'm assuming that the i18n locale is somehow set by the request locale?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds plausible :)
One general question...did you get a chance to see how this looks on smaller screens? |
I've added screenshots of other sizes to the description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Can't tell if the vertical spacing issue is totally fixed from screenshots or if they're just old, but don't think that should block it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
#homepage #actions .herolink-white i { | ||
vertical-align: middle; | ||
margin-right: 5px; | ||
font-size: 26px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, was 26px just the size that made things look right or was there a reason for picking this value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is specifically for the icon style i
, I just looked at the mocks in the ticket and it was the size that looked most like the icon in the mocks.
For en-US locale, display Celebrate Black History Month with us! link which links to
/blackhistorymonth
on the homepage.This was tested locally (couldn't find homepage tests to add to).
Links
Testing story
Reviewer Checklist: