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 Follow on Shop #2211

Merged
merged 7 commits into from
Jan 13, 2023
Merged

Add Follow on Shop #2211

merged 7 commits into from
Jan 13, 2023

Conversation

schaab
Copy link
Contributor

@schaab schaab commented Jan 11, 2023

PR Summary:

Add the Follow on Shop setting. Having this enabled creates a way for a merchants customers to follow their store in the Shop App. This allows the customer to get personalized product recommendations and notifications about the store.

Figma

Visual impact on existing themes

12-55-39ux9-nnr11

Testing steps/scenarios

The goal of testing is to ensure that the components show up in the proper position based on who their sibling components are. Then that it responds well to resizing of the browser window.

Make sure the Follow on Shop Button is enabled in the editor if it is not showing up in the store preview

See Figma for what alignment should look like

Browser

  • View theme in full width
  • Adjust the browser width for multiple sizes and ensure it positions correctly.

Mobile View Editor

In general content should stack and be centered.

  • Enable newsletter, follow on shop, social icons
  • Enable newsletter, social icons
  • Enable newsletter, follow on shop
  • Enable follow on shop, social icons
  • Enable newsletter
  • Enable follow on shop
  • Enable social icons

Desktop View

  • Enable newsletter, follow on shop, social icons
  • Enable newsletter, social icons
  • Enable newsletter, follow on shop
  • Enable follow on shop, social icons
  • Enable newsletter
  • Enable follow on shop
  • Enable social icons

Demo links

Checklist

@schaab schaab force-pushed the feat/follow-on-shop branch from 0ea44be to c44d785 Compare January 11, 2023 22:33
@schaab schaab self-assigned this Jan 11, 2023
@schaab schaab changed the title Feat/follow on shop Add Follow on Shop Jan 11, 2023
@metamoni
Copy link
Contributor

Running shopify theme check should help with the failing CI checks 🙂

LucasLacerdaUX
LucasLacerdaUX previously approved these changes Jan 13, 2023
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.

Looks good to me! :D

@schaab
Copy link
Contributor Author

schaab commented Jan 13, 2023

Running shopify theme check should help with the failing CI checks 🙂

I did run it and the "error" is related to a new liquid filter that we created. I documented it in this https://github.com/Shopify/storefront-renderer/pull/16606. As I understood it, this documentation fed the theme-check, however, what is opaque to me, is how long it takes to propagate there.

Add setting to allow merchants to turn on the Follow on Shop button.
This will allow customers to follow the store in the Shop app.
@schaab schaab force-pushed the feat/follow-on-shop branch from 3b4d275 to 5166a8a Compare January 13, 2023 17:22
LucasLacerdaUX
LucasLacerdaUX previously approved these changes Jan 13, 2023
@eugenekasimov eugenekasimov self-requested a review January 13, 2023 19:34
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.

For some reason FOLLOW ON SHOP is not displayed without SOCIAL MEDIA ICONS or EMAIL SIGNUP. Am I missing something?

Video.

@LucasLacerdaUX
Copy link
Contributor

Good catch, @eugenekasimov. I think we need to updte line 40 of footer.liquid:

  {%- if section.blocks.size > 0 or section.settings.newsletter_enable or section.settings.show_social -%}

should probably become:

  {%- if section.blocks.size > 0 or section.settings.newsletter_enable or section.settings.show_social or section.settings.enable_follow_on_shop -%}

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.

Im not sure if we would re work the focus outline on the button since it's not pulling the styles from the theme. But it looks like that on mobile:

Screenshot

Screenshot 2 with different outline color and spacing

align-items: flex-end;
margin-top: 3rem;
gap: 1rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I'm wondering if we should use the variables we already have instead of a hard coded value. The blocks coming up in the footer are using column-gap: var(--grid-desktop-horizontal-spacing); and column-gap: var(--grid-mobile-horizontal-spacing);.

Not a blocker, just a thought so everything is using a similar spacing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

13-50-eod8d-tayxp

I looked at this and it seems to break something.

Copy link
Contributor

@ludoboludo ludoboludo Jan 13, 2023

Choose a reason for hiding this comment

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

Oh maybe it's due to the flex-direction on mobile ? Since it's column 🤔
So it would need to be row-gap on mobile or use flex-wrap instead of flex-direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ludoboludo Good idea. I'll include this fix on my follow-up PR for footer.

If you're okay with everything else, let's ship this to main so we can include it as part of our next round of tests and address any minor styling issues as follow ups.

schaab and others added 2 commits January 13, 2023 12:37
Co-authored-by: Lucas Lacerda <37168033+LucasLacerdaUX@users.noreply.github.com>
@schaab
Copy link
Contributor Author

schaab commented Jan 13, 2023

Im not sure if we would re work the focus outline on the button since it's not pulling the styles from the theme. But it looks like that on mobile:

Screenshot
Screenshot 2 with different outline color and spacing

We have a PR open to try and solve this one https://github.com/Shopify/shop-js/pull/752.

Co-authored-by: Lucas Lacerda <37168033+LucasLacerdaUX@users.noreply.github.com>
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.

LGTM. Thanks for making the 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.

Looks good.

One minor thing I noticed, it's not really related to this PR, but if none of the Social Media Icons is assigned, it looks a bit weird and we can see shifting in layout effecting Follow on shop position. But as I said, maybe it's worth a separate PR.

@LucasLacerdaUX LucasLacerdaUX merged commit a5f65b1 into main Jan 13, 2023
@LucasLacerdaUX LucasLacerdaUX deleted the feat/follow-on-shop branch January 13, 2023 21:17
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
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.

5 participants