-
Notifications
You must be signed in to change notification settings - Fork 153
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
[BE/FE] Mozfest Session Slider #7515
Conversation
891f85e
to
8b5c762
Compare
This PR introduces visual differences. Click here to inspect the diffs. |
* video play on hover styling * Minor spacing * Add session carousel BE variables * Cleanup testing data * Move play triangle into template * End for loop * Session slider template location * Move styles into mozfest.scss to match event slider PR * Add settings to match events carousel * Adjust padding and grids of carousel * Adjust navigation styles * Add video image thumbnail for testing * Hook up video to session cards
This PR introduces visual differences. Click here to inspect the diffs. |
Hey @sabrinang and @Pomax! Whenever it's convenient, would you mind checking out what we have for the Session Slider? Thanks! |
This PR introduces visual differences. Click here to inspect the diffs. |
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.
Nothing really wrong, but there's some things we should consider with regards to our styleguide, including potentially filing issues for updates, before we merge this in.
network-api/networkapi/mozfest/templates/fragments/carousel/carousel_navigation.html
Outdated
Show resolved
Hide resolved
network-api/networkapi/mozfest/templates/fragments/carousel/carousel_navigation.html
Outdated
Show resolved
Hide resolved
network-api/networkapi/wagtailpages/templates/wagtailpages/blocks/session_slider_block.html
Outdated
Show resolved
Hide resolved
network-api/networkapi/mozfest/migrations/0023_add_session_slider.py
Outdated
Show resolved
Hide resolved
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.
Type/Color
- Title: font weight 600 (semi bold to match figma mockup)
- Session item title: bottom-margin 12px (so author subheading is closer, see figma mockup)
- @Pomax I don’t see color
#0d10bf
in the component visually but I do see it when I inspect the class — this can be replaced with#595cf3
to align or remove because I don’t see it on hover anyways if it is suppose to be. Thanks for pointing this out Pomax. - Regarding text sizes, let’s stick with https://foundation.mozilla.org/en/style-guide/#typography sizes as closely as we can but a separate issue where I need to sync with @fessehaye on tw-text sizes and colors if that is where the other blue came from
Button
- Button doesn’t show up for internal/external links
- Should the button be optional then? (currently required)
Links
- Internal/external links are ignored and links to the same home page unless a video is uploaded it goes to the video itself
Video
- When an image is uploaded and video is uploaded and I hover over the thumbnail, it is blank (it is blank if there is a video thumbnail present or not)
- When there is a link present and video, when I click on the heading or thumbnail it opens up the video on a separate page, I was thinking it was suppose to go to the link instead?
Thumbnails
- Does it need a play arrow for every thumbnail? Especially when there is no video present is this intentionally what the stakeholders want? (does the link always go to a video)
This PR introduces visual differences. Click here to inspect the diffs. |
* Add common reusable link blocks * BE: Mozfest Spaces Cards * [7430] FE - current events slider (#7505) * Current events slider styles * Install swiper and style event cards * Break slider out of container * Adjust spacing and containers for wide templates and signup sidebar * Merge FE on to BE * Update source/sass/mozfest.scss Co-authored-by: janehughes <janeelizabethhughes@gmail.com> Co-authored-by: janehughes <janeelizabethhughes@gmail.com> * Update template size condition * Grid adjustment depending on signup form being visible * Javascript cleanups as per code review * Update migrations * Make subheading_link optional * Carousel alignment fixes * Carousel full width * Signup form placement depending on which block is rendered in base * Add property to event block to check if wide * Add template tag for holding singup form boolean to render form * Js modifications as per feedback * Remove base page overrides for overflowing sliders, configure slider to show 2 cards on desktop * autoheight for carousels on mozfest * Fix flake issues * Fix eslint issues * Remove newline * Use double-quotes in tailwind config #7457 (comment) * Use template literal in querySelector https://github.com/mozilla/foundation.mozilla.org/pull/7457/files#r733242084 * Use forEach instead of for, use template literal for querySelectorAll https://github.com/mozilla/foundation.mozilla.org/pull/7457/files#r733241307 * Use single quotes #7457 (comment) * Remove unused load in mozfest-base * Remove unused template tag that was used in development * Remove empty carousel.scss * Remove duplicated class and fix nav title not showing with current events length * Remove commented out overlay * Switch colors to originally intended colors * Remove space changes * Update migrations * Replace ListBlock with StreamBlock * Remove rogue spacing changes * More rogue space changes * Minor spacing changes * Remove duplicate styles * Add padding to h2 * Remove newline change * Move navigation buttons into slider main container * Change title font weight to 600 #7457 (review) * Update subheading color to tw-text-blue (#595cf3) #7457 (review) * Remove fill-rule and clip-rule from carousel_navigation fragment #7515 (comment) * Use blocks instead of comments in block template #7515 (comment) * Update migrations * Remove tailwind config changes * Change mobile navigation active bullet color to #0d10bf #7457 (review) * Update migrations Co-authored-by: Steve Stein <steven_ts@hotmail.com> Co-authored-by: janehughes <janeelizabethhughes@gmail.com> Co-authored-by: Pomax <pomax@nihongoresources.com>
@b-ggs it looks like merging 7457 has created merge conflicts for this PR - just a few, but can you resolve those? I can merge it after that. |
This PR introduces visual differences. Click here to inspect the diffs. |
… feature/fe-be-mozfest-session-slider
This PR introduces visual differences. Click here to inspect the diffs. |
@Pomax, seems that The CI issues on this branch seems to stem from the migration issues on
|
@Pomax, looks like this is good to go! |
This PR introduces visual differences. Click here to inspect the diffs. |
This PR introduces visual differences. Click here to inspect the diffs. |
This PR introduces visual differences. Click here to inspect the diffs. |
@Pomax, remarking you as a reviewer to notify you that this should be good to merge! I've updated the migration to |
perfect, thank you |
Closes #7423
Implementation Checklist
Each item in slider contains:
Screenshots
Session Slider on a page with the signup snippet:
Session Slider on a page without the signup snippet:
Session Slider items can be localized:
Checklist