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

feat(Icons): add new icons #645

Merged
merged 3 commits into from
Aug 18, 2022
Merged

feat(Icons): add new icons #645

merged 3 commits into from
Aug 18, 2022

Conversation

PranjalMaithani
Copy link
Contributor

@PranjalMaithani PranjalMaithani commented Aug 18, 2022

Add new icons

  • ArrowRight
  • ArrowUpRight
  • ArrowDown
  • Search
  • Home

@changeset-bot
Copy link

changeset-bot bot commented Aug 18, 2022

🦋 Changeset detected

Latest commit: fc70825

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@razorpay/blade Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@divyanshu013 divyanshu013 left a comment

Choose a reason for hiding this comment

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

validate workflow is failing. Please run yarn test:react-native -u to update the snapshots for native as well.

Seems like some fill issue in home and search icons, you might have to check the plain svgs for this once:

image

image

@divyanshu013
Copy link
Contributor

Please also add the changeset

@@ -0,0 +1,20 @@
import { Svg, Path } from '../_Svg';
Copy link
Collaborator

Choose a reason for hiding this comment

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

If there is an issue with Home icon lets not add it here? Otherwise this PR will remain blocked

@@ -0,0 +1,20 @@
import { Svg, Path } from '../_Svg';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Search icon looks weird. Is this intended? Also couldn't find it on Figma. Can you point me to the correct

Screenshot 2022-08-18 at 3 21 10 PM

This is how it should be:
Screenshot 2022-08-18 at 3 24 15 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved now

Comment on lines 11 to 12
fill-rule="evenodd"
clip-rule="evenodd"
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 these should be fillRule, clipRule (TS should ideally be warning these). Similarly at other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This resolved the issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Created #648 to track this

Copy link
Contributor

@divyanshu013 divyanshu013 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.

"@razorpay/blade": patch
---

feat(Icons): add new icons
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
feat(Icons): add new icons
feat(Icons): add new icons
- ArrowDownIcon
- ArrowRightIcon
- ArrowUpIcon
- ArrowUpRightIcon
- HomeIcon
- SearchIcon

@chaitanyadeorukhkar chaitanyadeorukhkar merged commit f3abfbe into master Aug 18, 2022
@chaitanyadeorukhkar chaitanyadeorukhkar deleted the feat/new-icons branch August 18, 2022 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review - L2 Second level of review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants