Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow multiple announcement bars in Header group #2619

Merged
merged 2 commits into from
May 12, 2023
Merged

Allow multiple announcement bars in Header group #2619

merged 2 commits into from
May 12, 2023

Conversation

metamoni
Copy link
Contributor

@metamoni metamoni commented May 10, 2023 •
edited
Loading

PR Summary:

Allows merchants to add multiple Announcement bar sections to the Header group.

Why are these changes introduced?

Fixes #2583

What approach did you take?

  • added presents to announcement-bar section
  • tested theme update in spin

Visual impact on existing themes

When merchants click "Add section" in the Header group, they should be able to see an Announcement bar section. They should be able to add as many of these sections as they want, but only to the Header group.

Before
without-announcement-bar

After
with-announcement-bar

Testing steps/scenarios

  • Go to the theme editor
  • In the Header group, click Add section and verify that you see the Announcement bar section
  • Verify that you don't see the Announcement bar as an option in the Template and Footer groups
  • Verify that you can add multiple announcement bars to the Header
  • Change the settings of each announcement bar
  • Test on mobile and different browsers

Demo links

Checklist

@metamoni metamoni force-pushed the allow-multiple-announcement-bars branch from fc3938d to a5a0985 Compare May 10, 2023 15:45
@eugenekasimov eugenekasimov self-requested a review May 10, 2023 16:03
eugenekasimov
eugenekasimov approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@eugenekasimov eugenekasimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@melissaperreault melissaperreault self-requested a review May 11, 2023 13:10
@ludoboludo ludoboludo self-requested a review May 11, 2023 13:39
@melissaperreault
Copy link
Contributor

Unsure if you are able to replicate but this new ability made me uncover some lag and jumps on the page as I browse the navigation on smaller screen. Video reference (to watch at 0:25)

ludoboludo
ludoboludo approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Tested theme updates too and worked as I'd expect 👍

ludoboludo
ludoboludo reviewed May 11, 2023
View reviewed changes
sections/announcement-bar.liquid
}
]
}
]
Copy link
Contributor

@ludoboludo ludoboludo May 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure if you are able to replicate but this new ability made me uncover some lag and jumps on the page as I browse the navigation on smaller screen. Video reference (to watch at 0:25)

I think this is due to the fact that those are the first elements in the view of the page and they need a bit of JS to get everything setup. So there might be a bit of flash like you noticed. Something we could look into and potentially fix by moving some CSS around and making sure it's loaded as soon as possible to prevent what you saw. It might be easier to test by simulating a slow network

Copy link
Contributor Author

@metamoni metamoni May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@melissaperreault I'm struggling to replicate the behaviour in your video, where the font initially appears bigger, but I do see a slight jump. I have noticed it on main as well, though, so I don't think this issue was introduced by the stacked announcement bars. Here's a slow motion video of what I'm seeing on main.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks! We can open an issue to investigate further!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New issue created here: #2632

@metamoni metamoni force-pushed the allow-multiple-announcement-bars branch from 648cbf0 to a5a0985 Compare May 12, 2023 09:27
@metamoni metamoni merged commit 6118e89 into main May 12, 2023
@metamoni metamoni deleted the allow-multiple-announcement-bars branch May 12, 2023 15:34
ludoboludo pushed a commit that referenced this pull request May 12, 2023
@metamoni @ludoboludo
Allow multiple announcement bars in Header group (#2619)
e7df89b 
allow multiple announcement bars in header
ludoboludo pushed a commit that referenced this pull request May 12, 2023
@metamoni @ludoboludo
Allow multiple announcement bars in Header group (#2619)
7be25e9 
allow multiple announcement bars in header
@translation-platform translation-platform bot mentioned this pull request May 15, 2023
Merged
ludoboludo pushed a commit that referenced this pull request Jun 5, 2023
@metamoni @ludoboludo
Allow multiple announcement bars in Header group (#2619)
c937c9a 
allow multiple announcement bars in header
pangloss added a commit to pangloss/dawn that referenced this pull request Jun 14, 2023
@pangloss
Merge remote-tracking branch 'shopify/main' into next
bb855f2 
* 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)
  ...
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
@metamoni
Allow multiple announcement bars in Header group (Shopify#2619)
0408b38 
allow multiple announcement bars in header
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ludoboludo ludoboludo ludoboludo approved these changes

@melissaperreault melissaperreault melissaperreault approved these changes

@eugenekasimov eugenekasimov eugenekasimov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Navigation UX] Allow Announcement bar to be added multiple times (stacked)
4 participants
@metamoni @melissaperreault @ludoboludo @eugenekasimov