-
Notifications
You must be signed in to change notification settings - Fork 146
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(blade): add Breadcrumb
component
#2028
Conversation
🦋 Changeset detectedLatest commit: 5679436 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
✅ PR title follows Conventional Commits specification. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 5679436:
|
There was a problem hiding this 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
@anuraghazra Keep home icon only on the first item in this story? This looks weird |
} | ||
|
||
return ( | ||
<BaseLink |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets add href and rel here by default? Or baselink will add it if we don't mention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
href will already be passed by consumer, rel is not needed.
rel is only needed if we had target, to add rel="noreferrer noopener"
@@ -78,6 +78,7 @@ type BaseLinkCommonProps = { | |||
* The title of the link which is displayed as a tooltip. This is a web only prop and has no effect on react-native. | |||
*/ | |||
htmlTitle?: string; | |||
opacity?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are changing the opacity? Can't we use a gray color that is lighter instead? Our color already has an alpha color and on top of that we're changing it more. This just means that our colors aren't sufficing our needs if we need to use opacity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added same comments on figma, to be discussed with RK on monday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this discussion done? Can we move away from opacity?
* test: add breadcrumb tests * chore: add breadcrumb kitchen sink * chore: update snaos
Bundle Size ReportUpdated Components
|
Breadcrumb
component
"@razorpay/blade": minor | ||
--- | ||
|
||
feat(blade): breadcrumb web implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feat(blade): breadcrumb web implementation | |
feat(blade): add `Breadcrumb` component |
* feat: add breadcrumb implementation * chore: add docs * chore: update native files * refactor: use react context * chore: add meta * chore: update docs * chore: test * chore: reexport * Create five-cows-cheer.md * chore: add multiline example & fix spacing * chore: update * chore: update types * chore: update opacity token * test: add breadcrumb tests (#2029) * test: add breadcrumb tests * chore: add breadcrumb kitchen sink * chore: update snaos * chore: update story * chore: update code url
Description
Changes
Additional Information
Component Checklist