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 Shopify Finance icons #12233

Merged
merged 3 commits into from
Jul 18, 2024
Merged

Add Shopify Finance icons #12233

merged 3 commits into from
Jul 18, 2024

Conversation

ngkay
Copy link
Contributor

@ngkay ngkay commented Jun 11, 2024

WHY are these changes introduced?

Fixes #12171

WHAT is this pull request doing?

Adds new icons for Shopify Finance
Figma - the icons marked "Ready for dev"
Project

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

@ngkay ngkay marked this pull request as draft June 11, 2024 18:21
@ngkay ngkay force-pushed the kn/finance-icons branch 2 times, most recently from b6d0301 to 11e55a8 Compare June 11, 2024 18:52
@alex-page
Copy link
Member

Hey @ngkay what is the urgency behind this icon addition.

@ngkay
Copy link
Contributor Author

ngkay commented Jun 12, 2024

Hey @ngkay what is the urgency behind this icon addition.

hey @alex-page, product and UX find this an important part of the the Finances Unification so we want to have this for them ASAP

@ngkay ngkay force-pushed the kn/finance-icons branch 6 times, most recently from 7bb9c1a to 49c1db1 Compare June 14, 2024 18:18
@ngkay ngkay marked this pull request as ready for review June 14, 2024 19:05
@alex-page
Copy link
Member

@ngkay totally understand. Just wanted to know if it was needed for Editions? If not we will add this icon post editions.

@ngkay
Copy link
Contributor Author

ngkay commented Jun 17, 2024

@ngkay totally understand. Just wanted to know if it was needed for Editions? If not we will add this icon post editions.

@alex-page would after Editions be the first week of July or later?

@alex-page
Copy link
Member

alex-page commented Jun 18, 2024

It would be in July. I can't give accurate weekly estimates.

Copy link
Contributor

@heyjoethomas heyjoethomas left a comment

Choose a reason for hiding this comment

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

I think a couple of the suggestions Alex is making are valid and we could leverage existing icons for refund and purchase.

@mujicaed
Copy link

mujicaed commented Jul 15, 2024

@heyjoethomas @ngkay @alex-page let's use the receipt icon for purchases and refunds ✅

Screenshot 2024-07-15 at 4 53 31 PM

@ngkay
Copy link
Contributor Author

ngkay commented Jul 15, 2024

@heyjoethomas @ngkay @alex-page I'm ok to move forward using the receipt icon for purchases and refunds.

@mujicaed it looks like the actual Receipt icon is slightly different
Screenshot 2024-07-15 at 4 56 17 PM
but I can take out the purchases and refunds icons from here and use the existing ones 👍

@alex-page
Copy link
Member

@heyjoethomas the ReceiptIcon looks to be misaligned across Figma and code. Should we update it as a part of this work?

@heyjoethomas
Copy link
Contributor

@heyjoethomas the ReceiptIcon looks to be misaligned across Figma and code. Should we update it as a part of this work?

Yeah. I can help make sure that happens.

@heyjoethomas
Copy link
Contributor

Figma has been updated to reflect what is in this repository.

@alex-page
Copy link
Member

@heyjoethomas thank you! I was wondering if we should use the old design in Figma here. Sounds like you made a call to just align them. That is all good but just saying we can update it here too if you wanted that design.

@ngkay ngkay merged commit 491cf80 into main Jul 18, 2024
7 of 8 checks passed
@ngkay ngkay deleted the kn/finance-icons branch July 18, 2024 17:51
alex-page pushed a commit that referenced this pull request Jul 22, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @shopify/[email protected]

### Minor Changes

- [#12233](#12233)
[`491cf8038`](491cf80)
Thanks [@ngkay](https://github.com/ngkay)! - Adds new icons for Shopify
Finance

## @shopify/[email protected]

### Minor Changes

- [#12359](#12359)
[`8f0f1b7e0`](8f0f1b7)
Thanks [@mrcthms](https://github.com/mrcthms)! - Removed all references
to the dynamicTopBarAndReframe feature and revert functionality back to
how it was

### Patch Changes

- Updated dependencies
\[[`491cf8038`](491cf80)]:
    -   @shopify/[email protected]

## [email protected]

### Patch Changes

- Updated dependencies
\[[`8f0f1b7e0`](8f0f1b7),
[`491cf8038`](491cf80)]:
    -   @shopify/[email protected]
    -   @shopify/[email protected]

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@heyjoethomas
Copy link
Contributor

heyjoethomas commented Jul 23, 2024

@alex-page That is because we have a bunch of variants of the Receipt icon. So if we change this one we would need to change them all and I don't think that level of effort is worth it here.
Screenshot 2024-07-23 at 10 10 02 AM

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.

[Icons]: New icons for Shopify Finance
6 participants