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

Add top border on cart notification when "show separator line" setting is deactivated #306

Merged
merged 2 commits into from
Aug 4, 2021

Conversation

tauthomas01
Copy link
Contributor

Why are these changes introduced?

Fixes #278

The goal of this PR is to add a border top on the cart notification even when the header Show separator line setting is deactivated.

Show separator line - Deactivated

alt

Show separator line - Activated

alt

What approach did you take?

  • Add CSS when the header .header-wrapper--border-bottom class exist.

Other considerations

  • We could add the "show seperator line" setting as a parameter when referencing the {% render cart-notification %} snippet, but I thought it would be better to leverage native CSS, even though the selector looks more complicated.

Demo links

Checklist

tyleralsbury
tyleralsbury previously approved these changes Aug 3, 2021
Copy link
Contributor

@tyleralsbury tyleralsbury left a comment

Choose a reason for hiding this comment

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

Tested with the separator enabled disabled and it looks good in both cases.

Comment on lines 24 to 28
.header-wrapper:not(.header-wrapper--border-bottom)
+ cart-notification
.cart-notification {
border-top-width: 0.1rem;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.header-wrapper:not(.header-wrapper--border-bottom)
+ cart-notification
.cart-notification {
border-top-width: 0.1rem;
}
.header-wrapper:not(.header-wrapper--border-bottom) + cart-notification .cart-notification {
border-top-width: 0.1rem;
}

I find this easier to read, but I'm not strongly opinionated on the matter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same. For a second I thought this was using some kind of nesting haha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the changes. It was Prettier VS code plugin who does the autoformatting so I disabled it.

LucasLacerdaUX
LucasLacerdaUX previously approved these changes Aug 3, 2021
Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX left a comment

Choose a reason for hiding this comment

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

🐕 ✅

@tauthomas01 tauthomas01 dismissed stale reviews from LucasLacerdaUX and tyleralsbury via 4272f86 August 3, 2021 19:40
@tauthomas01 tauthomas01 force-pushed the fix-cart-notification-border branch from c041d81 to 4272f86 Compare August 3, 2021 19:40
@kmeleta kmeleta self-requested a review August 4, 2021 19:40
Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@tauthomas01 tauthomas01 merged commit e3ea034 into main Aug 4, 2021
@tauthomas01 tauthomas01 deleted the fix-cart-notification-border branch August 4, 2021 19:49
olafghanizadeh pushed a commit to hyttefeber/dawn that referenced this pull request Aug 5, 2021
…g is deactivated (Shopify#306)

* Add border on cart notification when header border setting is not active

* adjust presentation of the selector
maplerock added a commit to fellowsmedia/nsra_shopify_2021 that referenced this pull request Aug 12, 2021
* main: (23 commits)
  Cart content update (Shopify#370)
  Footer spacing and alignment adjustments (Shopify#369)
  Product Template UI polish updates (Shopify#219)
  footer ui updates (Shopify#318)
  Fix cart improvements empty state (Shopify#319)
  [Announcement] Adjust block id for displaying dynamic names (Shopify#327)
  Update translations: buyer (Shopify#329)
  Revert editor setting changes (Shopify#328)
  Update translations (Shopify#294)
  Fix collection filtering UX (Shopify#268)
  Added page width setting and fixed image quality (Shopify#292)
  Add top border on cart notification when "show separator line" setting is deactivated (Shopify#306)
  movebadge code into base.css (Shopify#313)
  Collage UI bug fixes (Shopify#308)
  Customer account UI polish (Shopify#177)
  Added custom liquid block to product page (Shopify#269)
  Fix disclosure icon (Shopify#310)
  Fix facet price filter label spacing (Shopify#300)
  Fix duplicate search icon when header logo is set to "Top center" (Shopify#252)
  Add card outline setting (Shopify#239)
  ...
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
…g is deactivated (Shopify#306)

* Add border on cart notification when header border setting is not active

* adjust presentation of the selector
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.

Cart Notification Missing top border
4 participants