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

Update HelpText to maintain parity with what is currently in web #23

Merged
merged 5 commits into from
Jul 11, 2022

Conversation

knames
Copy link
Contributor

@knames knames commented Jul 5, 2022

Updated HelpText combo card to match what is currently on Web.

Figma: https://www.figma.com/file/m81kpkSdIYieWQQA9rl5qr/Extensible-Discounts-Main-File?node-id=8053%3A142722#215788222

Work done in Web: https://github.com/Shopify/web/pull/67404/files#diff-d19bbdbc75c3532233b29c7d18d337618cf24497545a03b7bef3693fce4429f9

What problem is this PR Solving?

Addressing the feedback from Glen and UX team for combinations card

https://github.com/Shopify/web/commit/c7ccafb42c1c789cfa300c6bb1c8a9d4b1ef8984 Empty state changes
Update copy
Remove icon and update the layout
https://github.com/Shopify/web/commit/033384ae6347941902722119536082db0d1b5087 Combinations changes
Update CTA content to have a special case for product combines with product scenario
Update main copy to only have warning about overdiscounting when product combines with product

Resolves https://github.com/Shopify/core-issues/issues/40640

Reviewer Hatrack:

orderDiscounts: false,
    productDiscounts: false,
    shippingDiscounts: true,

and

 combinableDiscountCounts={{
          orderDiscountsCount: 0,
          productDiscountsCount: 3,
          shippingDiscountsCount: 0,
        }}

and

        discountClass={DiscountClass.Product}

to run through the different states/copy

Product with Product and Shipping combos image
Product with No combos image

What is the impact of this PR?

Make sure you consider Shopify experience values and any consequences on Merchants or on Shopify.

On Merchants

On Shopify

Before you deploy

  • This PR is safe to rollback. ⚠️ This field is required.1
  • I tophatted this change.2
    • I tophatted this change with a fresh login.3
  • I tophatted this change in staging if it is a risky change that requires additional testing in a safe environment before shipping to canaries.2
  • I tophatted this change with production assets if it meaningfully changes files in the /foundation or /server directories, or if it updates a key dependency like Polaris or sewing kit.

Footnotes

  1. If it is not safe, please detail what would need to happen to make it safe. For example, if this PR removes a GraphQL field that will then be removed from the schema, link to the Shopify Core PR that removes the field.

  2. If you are having difficulties accessing or deploying to staging, or you are having difficulties running the production build, please ask in #shopify-web. 2

  3. Instead of just refreshing when switching between production/staging/canaries, consider performing a fresh login, especially when your changes touch configuration or the server middleware.

@ghost ghost added cla-needed and removed cla-needed labels Jul 5, 2022
@knames knames force-pushed the ks-combo-ux-updates-40640 branch from 9dfe781 to 9ccece8 Compare July 6, 2022 00:39
@knames knames marked this pull request as ready for review July 6, 2022 00:39
@knames knames requested a review from a team July 6, 2022 00:39
@knames knames force-pushed the ks-combo-ux-updates-40640 branch from 074e3bc to 8f02ffc Compare July 6, 2022 17:07
<Link
url={`https://help.shopify.com/${i18n.locale}/manual/discounts/combining-discounts`}
external
// todo: do we want to import @web-utilities?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so, but I wouldn't be the one to make the call. I think web-utilities has a lot of private code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed! This comment can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh hey derek commented on this i forgot to remove the comment. thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh and there's derek again, thanks!

Copy link
Contributor

@devisscher devisscher left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@carrotderek carrotderek left a comment

Choose a reason for hiding this comment

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

🎩 and code lgtm! Just waiting for translations to come back.

@knames knames force-pushed the ks-combo-ux-updates-40640 branch from 623d215 to 69e65a6 Compare July 6, 2022 20:38
</Stack>
) : (
<>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't think you need to use a fragment here

Copy link
Contributor

@lone-star lone-star left a comment

Choose a reason for hiding this comment

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

Didn't 🎩 , but code is 👍

Ken Slawinski and others added 5 commits July 11, 2022 11:51
@knames knames force-pushed the ks-combo-ux-updates-40640 branch from 332af38 to 181ba27 Compare July 11, 2022 17:51
@knames knames merged commit f91c53d into main Jul 11, 2022
@knames knames deleted the ks-combo-ux-updates-40640 branch July 11, 2022 19:20
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.

None yet

4 participants