AI Launch - Landing page redesign - #51191
Conversation
|
One thing I'm noticing: on small screens, the teachers' quotes aren't coming through |
@megcrenshaw it works if you reload the page and it's already in mobile — I assume this has something to do w/ the jquery plugin that’s used not adjusting the height on a browser resize. |
| @@ -0,0 +1,3 @@ | |||
| <div> | |||
| <%=view :display_video_thumbnail, id: "why_ai_matters", video_code: "dWRnCXbUDgA", play_button: 'center', letterbox: 'false', download_path: "//videos.code.org/ai_series/ai-why-ai-matters.mp4" %> | |||
There was a problem hiding this comment.
The display_video_thumbnail view isn't tab-navigable, i.e. I can't hit "play" using a keyboard. What I don't know is the best way to fix that. Maybe in the display_video_thumbnail partial, this img tag
%img.play{:src => "/shared/images/play-button.png", :alt => "play button"}
needs to be wrapped in an anchor or button tag? https://stackoverflow.com/questions/23475534/make-img-tag-tabbable-respond-to-tab-key-navigation
Looks like YouTube uses an a tag, but that goes to another page ...
There was a problem hiding this comment.
The display_video_thumbnail partial is used all over the site — I think a fix like this would be better in it's own issue/PR so we can adequately test it out. I'll make a note of this!
There was a problem hiding this comment.
|
|
||
| var id = '#ai-carousel'; | ||
| // Define the carousel using the ID. | ||
| $(id + ' .slides').carouFredSel({ |
There was a problem hiding this comment.
This actually read on VoiceOver really nicely! But I wasn't able to tab navigate to the next slide. I'm not familiar with the carouFredSel plugin and a quick search didn't pull up an easy fix for this –– I don't think this is a blocker
There was a problem hiding this comment.
In the future I'd like to replace this plugin w/ either a native JS slider or a non-jQuery slider so we have more control over stuff like this, but this one is already used throughout the site.
| .wrapper | ||
| .text-wrapper.col-60 | ||
| %h1{style: "color: white; margin-bottom: 0"}=hoc_s(:ai_hero_heading_01) | ||
| %h2.heading-xxl{style: "color: white"}=hoc_s(:ai_hero_heading_02) |
There was a problem hiding this comment.
Is there a reason why the first part is an h1 and the second an h2? It reads a bit funky on VoiceOver due to the different header levels.
There was a problem hiding this comment.
I did it so we could get an intentional line break on the "It's just code!" line and wanted to avoid two h1s — but maybe it's better to try to break this up w/ css and have the whole string as one string for better translation?
There was a problem hiding this comment.
@mgc1194 maybe you can speak to what would be better here? These are the two strings in question:
code-dot-org/pegasus/sites.v3/hourofcode.com/i18n/en.yml
Lines 149 to 150 in d000dfd
There was a problem hiding this comment.
I do think for translatability, it'd be better to keep this as one string. But I'm good with it being a follow-up, as I know getting the styling right will be tricky
Ah, I see! Yes, this looks good –– thanks! |
mgc1194
left a comment
mgc1194
left a comment
There was a problem hiding this comment.
So much work!
I really appreciate that all new strings are already localizable!
megcrenshaw
left a comment
megcrenshaw
left a comment
There was a problem hiding this comment.
Having a single header string would I think be more translatable and more accessible, but it's not urgent. This looks great! Thanks for making this easy to review.
4 participants
Redesign https://code.org/ai with the new brand styles.
display_video_thumbnailpartial pattern.Asana task: https://app.asana.com/0/0/1204240637970988
Animated hero banner graphic
Screen.Recording.2023-04-13.at.1.20.47.PM.mov
Before
After