-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[Announcement bar] Add social icons #2497
Conversation
I am against renaming this section to utility bar. The naming announcement-bar is used consistently in Shopify ecosystem. Renaming Dawn section to utility bar now would create a big inconsistency with our themes that have been using announcement bar for ages. Renaming this would also break most of existing resources, tutorials and documentation. |
Add basic logic for social-icons in the utility-bar.
530fc50
to
bdcf2c6
Compare
Remove social icons snippet
Change the slider width for screens wider than 1200px. Rename back a file name from utility-bar to announcement-bar.
@eugenekasimov to summarize the changes :
For the announcement slider :
What is left for us to decide :
|
Reduce social icons sizes for in the utility bar. Change grid system.
@eugenekasimov an update about the utility bar. We decided to descope the secondary navigation (at any level of the header group). So we don't see much value into doing the 2 rows layout on minimum desktop anymore. Do you think we can remove that behavior easily? I also updated the figma file. 1/ If there is no announcements but we have social media icons, we can have all the icons on the row. |
@YoannJailin thanks for update 👌. |
…der 1120px. Remove JS logic.
407dd03
to
e224d31
Compare
Refactor of liquid logic. Use social-icons as a snippet.
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.
A few nitpicks but it's looking good 👍
sections/announcement-bar.liquid
Outdated
{%- endif -%} | ||
{%- if section.blocks.size == 1 -%} | ||
<div | ||
class="announcement-bar{% if section.settings.show_social %} utility-bar__item--announcement{% endif %}" |
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 find the class name utility-bar__item--announcement
not quite right. It's a modifier and would need to be applied along with a utility-bar__item
class to follow BEM convention.
I notice it's already the case on other elements (eg: utility-bar--grid
) which could be addressed too.
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 find the class name
utility-bar__item--announcement
not quite right. It's a modifier and would need to be applied along with autility-bar__item
class to follow BEM convention.
Oh, I guess your point is that there is no element with class .utility-bar__item
so it's not right to use .utility-bar__item--announcement
on its own. Is it right?
How do you feel if I change this .utility-bar__item--announcement
to .announcement-bar--one-announcement
because basically I need to apply this class when the quantity of announcements = 1.
Change the naming in css accordingly BEM.
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 think it's look good 🤔 👍
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.
Looks great! Added a few small questions 👀
@@ -99,7 +99,7 @@ html.no-js .no-js-hidden { | |||
padding: 0 1.5rem; | |||
} |
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.
Shouldnt it hide on mobile if the setting is off? 👀 https://screenshot.click/02-27-h1a02-slcfh.mp4
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.
It's the same behaviour we have with the footer section, no matter if you show social icons there or not, we always display them in the drawer.
I feel it may make sense to work on this in the feature, but it should not be a blocker for this PR. I don't remember the context, but it's hard to decide what logic to follow when you have social-icons in three places: the announcement bar, footer and drawer. And for example, if you want to show them in the footer but not in the announcement-bar, then should we show them in the drawer? On top of that, since social-icons live in three different sections we cannot make social-icons in the header to be dependent on settings in the announcement bar or footer.
|
||
@media screen and (max-width: 989px) { | ||
.utility-bar .utility-bar__grid .list-social { | ||
display: none; |
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.
If we dont show it on mobile, shouldnt we add a note to the settings? 🤔
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.
Thanks for brining it up. In my opinion we can assume it's something intuitive, however if you feel we should raise this question, I'm okay with that.
@YoannJailin what do you think about the fact that show social-icons on the screen size up to 990px and for smaller screens we hide them because we always show icons in the drawer? Do we mention about it in the settings?
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 could see how some info text could help understand that when it's mobile/tablet size it will be moved in the menu drawer.
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.
@YoannJailin @melissaperreault could you pls step in and help us decide? I can easily add info text, I just need UX approval for this :)
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.
@eugenekasimov i agree we should be explicit that some features only shows up on mobile. Let me take a look on that this afternoon.
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.
@sofiamatulis we decided to change the label to Show icons on desktop
. Thanks for brining up this question.
fixed here dc6dd49
@media screen and (min-width: 990px) { | ||
.utility-bar__grid { | ||
grid-template-columns: 3fr 4fr 3fr; | ||
grid-template-areas: 'social-icons announcements language-currency'; |
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 dont see language-currency
in the announcement bar. Am I missing something? 👀
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.
it's going to be a next feature I'll be working on for the announcement bar. But since I created grid system in this PR I decided to name it properly already now.
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.
Looks good :) thanks for the changes 💯
* shopify/main: (59 commits) [Announcement bar] Add social icons (Shopify#2497) Update theme version to match the pubic release (Shopify#2698) Add release/v10.0.0 branch fixes to main (Shopify#2693) fix default UI for dropdown and mega menu (Shopify#2644) Fix link formatting in Related Products heading (Shopify#2680) Update 2 translation files (Shopify#2637) Enable gift card recipient form by default on featured product section (Shopify#2666) Gift cards/enable recipient form by default (Shopify#2618) Add a Color Scheme setting for Menus-Header (Shopify#2622) Made mobile drawer full width by default-header (Shopify#2625) Allow multiple announcement bars in Header group (Shopify#2619) [Feat Product] Add rating styling sheet (Shopify#2620) Fix password page variables (Shopify#2607) Fix transform applied when it should not for sliders (Shopify#2606) Modify info string for gift card recipient checkbox (Shopify#2588) [3D lift animation] Raise hovered card above adjacent cards (Shopify#2589) Remove fallback color scheme info text (Shopify#2602) Fix CSS specifity issue to cancel animation for theme editor events (Shopify#2605) Remove settings daya for icon color (Shopify#2601) Follow ups for accessibility of announcements slider (Shopify#2580) ...
* Add animation to the announcement bar. * Add animation for the current and next slides. * Make animation works for auto-rotating. * Ensure the slider rotates in reverse when it reaches the end/begining. * Add condition to apply delay for announcement-bar only * Refactor and update some logic. * Refactoring. * Remove unneccessary css code. * Refactor css. Change the order. * Refactor JS. * Refactor css. * Isolate all logic in slideshowComponent. * Simplify applyAnimation method. * Reuse currentPage * Apply animation on selected slides. * Adjust duration of animation and shifting. * Refactoring * Another refactoring * Remove timeout and add delay in css * Refactoring * Add no-js * Prevent snapping during animation * Fix auto-rotation positioning * Refactor applyAnimation * Refactor and remove unused elements * [Announcement bar] Add social icons (#2497) * Rename announcement-bar to utility-bar section. Add basic logic for social-icons in the utility-bar. * Remove social icons from the utility bar for tablet Remove social icons snippet * Prevent auto-rotation after users interact with arrow buttons. Change the slider width for screens wider than 1200px. Rename back a file name from utility-bar to announcement-bar. * Prevent auto-rotation for mobile. Reduce social icons sizes for in the utility bar. Change grid system. * Prevent showing bottom-border when the utility bar is empty. * Add layout for screens under 1200px. * Adjust a separater line for max-width: 1199px * Revert announcement-bar name. Remove additional layout for desktop under 1120px. Remove JS logic. * Minor changes. * Refactor of css classes. Refactor of liquid logic. Use social-icons as a snippet. * Refactor in social-icons snippet. Change the naming in css accordingly BEM. * Update 14 translation files * Update 6 translation files * Change label for show_social. * Update 15 translation files * Update 5 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> * Use live region to announce recipient form (#2672) * Use live region to announce recipient form * Inject text instead of toogling aria-hidden * Announce form collapsed when checkbox unticked * Update 20 translation files * Update 4 translation files * Update 6 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> * Safari fix and addressing feadback * Correct typo * Remove unnecessary code * Prevent applying animation when you scroll * Remove console.log * Refactoring --------- Co-authored-by: Ludo <[email protected]> Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Co-authored-by: Fred Ma <[email protected]>
* Rename announcement-bar to utility-bar section. Add basic logic for social-icons in the utility-bar. * Remove social icons from the utility bar for tablet Remove social icons snippet * Prevent auto-rotation after users interact with arrow buttons. Change the slider width for screens wider than 1200px. Rename back a file name from utility-bar to announcement-bar. * Prevent auto-rotation for mobile. Reduce social icons sizes for in the utility bar. Change grid system. * Prevent showing bottom-border when the utility bar is empty. * Add layout for screens under 1200px. * Adjust a separater line for max-width: 1199px * Revert announcement-bar name. Remove additional layout for desktop under 1120px. Remove JS logic. * Minor changes. * Refactor of css classes. Refactor of liquid logic. Use social-icons as a snippet. * Refactor in social-icons snippet. Change the naming in css accordingly BEM. * Update 14 translation files * Update 6 translation files * Change label for show_social. * Update 15 translation files * Update 5 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
* Add animation to the announcement bar. * Add animation for the current and next slides. * Make animation works for auto-rotating. * Ensure the slider rotates in reverse when it reaches the end/begining. * Add condition to apply delay for announcement-bar only * Refactor and update some logic. * Refactoring. * Remove unneccessary css code. * Refactor css. Change the order. * Refactor JS. * Refactor css. * Isolate all logic in slideshowComponent. * Simplify applyAnimation method. * Reuse currentPage * Apply animation on selected slides. * Adjust duration of animation and shifting. * Refactoring * Another refactoring * Remove timeout and add delay in css * Refactoring * Add no-js * Prevent snapping during animation * Fix auto-rotation positioning * Refactor applyAnimation * Refactor and remove unused elements * [Announcement bar] Add social icons (Shopify#2497) * Rename announcement-bar to utility-bar section. Add basic logic for social-icons in the utility-bar. * Remove social icons from the utility bar for tablet Remove social icons snippet * Prevent auto-rotation after users interact with arrow buttons. Change the slider width for screens wider than 1200px. Rename back a file name from utility-bar to announcement-bar. * Prevent auto-rotation for mobile. Reduce social icons sizes for in the utility bar. Change grid system. * Prevent showing bottom-border when the utility bar is empty. * Add layout for screens under 1200px. * Adjust a separater line for max-width: 1199px * Revert announcement-bar name. Remove additional layout for desktop under 1120px. Remove JS logic. * Minor changes. * Refactor of css classes. Refactor of liquid logic. Use social-icons as a snippet. * Refactor in social-icons snippet. Change the naming in css accordingly BEM. * Update 14 translation files * Update 6 translation files * Change label for show_social. * Update 15 translation files * Update 5 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> * Use live region to announce recipient form (Shopify#2672) * Use live region to announce recipient form * Inject text instead of toogling aria-hidden * Announce form collapsed when checkbox unticked * Update 20 translation files * Update 4 translation files * Update 6 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> * Safari fix and addressing feadback * Correct typo * Remove unnecessary code * Prevent applying animation when you scroll * Remove console.log * Refactoring --------- Co-authored-by: Ludo <[email protected]> Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Co-authored-by: Fred Ma <[email protected]>
PR Summary:
This PR:
Show social media icons
to the announcement bar.Why are these changes introduced?
Fixes #399.
What approach did you take?
Other considerations
We have social-icons snippet but it has a css class related to the footer. I think as a follow up later we should adjust and make the snippet more universal to use it in the drawer and the utility-bar.
Visual impact on existing themes
Social-icons + announcements.
Announcements only.
Social-icons only.
Testing steps/scenarios
Show social media icons
. Make sure that the width of the slider changes from 60% to 40%.Demo links
Checklist