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.

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] Change the slider width to 60% #2593

Merged
merged 4 commits into from
May 1, 2023

Conversation

eugenekasimov
Copy link
Contributor

@eugenekasimov eugenekasimov commented Apr 28, 2023

PR Summary:

This PR changes the announcement bar slider with to 60%.

Why are these changes introduced?

This change was a part of another PR, however due to lack of time we won't be able to review that PR before the upcoming release. We want to reduce the width to avoid that too prominent animation that we currently have. We'll add a more elegant animation to the announcement bar slider as a next step.

What approach did you take?

Other considerations

Decision log

# Decision Alternatives Rationale Downsides
1

Visual impact on existing themes

Testing steps/scenarios

  • Add a few announcements to the announcement bar and make sure that the width of the slider is 60% of the header.
  • Change the menu to the drawer and make sure that the width of the slider changes accordingly.

Demo links

Checklist

@eugenekasimov eugenekasimov self-assigned this Apr 28, 2023
@eugenekasimov eugenekasimov force-pushed the change-announcement-bar-slider-size branch from a1a89ae to ccc8f76 Compare April 28, 2023 22:12
@melissaperreault melissaperreault self-requested a review April 29, 2023 00:40
assets/base.css Outdated
Comment on lines 2321 to 2322
.announcement-bar-slider {
width: 60%
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this fixed the issue on mobile but now on large screen it doesn't look at 60% visually

Copy link
Contributor

Choose a reason for hiding this comment

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

@melissaperreault I'm confused about the screenshot. The portion you highlighted looks like 40% to me, rather than 60%. What @eugenekasimov did looks right to me 🤔

Copy link
Contributor

@melissaperreault melissaperreault May 1, 2023

Choose a reason for hiding this comment

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

👀 You are so right! Sorry for the confusion, I guess it was my Friday brain! 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@melissaperreault So, everything looks okay 😅 ?

@kmeleta kmeleta self-requested a review May 1, 2023 15:32
Copy link
Contributor

@melissaperreault melissaperreault left a comment

Choose a reason for hiding this comment

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

LGTM! 🚢

Co-authored-by: Ken Meleta <30790058+kmeleta@users.noreply.github.com>
@eugenekasimov eugenekasimov merged commit e1d93cd into main May 1, 2023
@eugenekasimov eugenekasimov deleted the change-announcement-bar-slider-size branch May 1, 2023 21:55
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
* Change the slider width to 60%

* Change the slider width to 60%

* Add missing ';'

Co-authored-by: Ken Meleta <30790058+kmeleta@users.noreply.github.com>

---------

Co-authored-by: Ken Meleta <30790058+kmeleta@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants