-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
creating story for cancel button
#19566
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
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.
Thanks for helping out @rohiiittttt
Our old Button is not a 1 to 1 transition to the new Button. I've made a few comments on changes to closely align.
But upon looking at this and with the recommended changes, there still might be some concerns about the font size of our new button when next to an old button.
This will begin to make this PR a bit larger since now the Primary buttons paired next to the CancelButton also need to be updated to the latest version.
With that being all said, you are still welcome to add the cancel-button.stories.js
file and undo any changes to cancel-button.js
}, | ||
}, | ||
args: { | ||
detailsModal: 'true', |
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.
This needs to be a boolean, not a string
detailsModal: 'true', | |
detailsModal: true, |
@@ -3,7 +3,7 @@ import React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import { useSelector } from 'react-redux'; | |||
import classnames from 'classnames'; | |||
import Button from '../../ui/button'; | |||
import { Button } from '../../component-library'; |
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.
The new Button component is not a 1 to 1 transition from the old one. Instead of using type
, it now uses variant
. Need to use BUTTON_VARIANT
to change the variant of this Button.
import { Button } from '../../component-library'; | |
import { Button, BUTTON_VARIANT } from '../../component-library'; |
Then for the actual Button you will need to change
type="secondary"
to variant={BUTTON_VARIANT.SECONDARY}
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.
@garrettbear i have made the changes please check..
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.
Hey @rohiiittttt, thanks for your contribution but in this case updating the component here effects other UI so I'm reverting those changes and keeping the story. I think updating this components deprecated components should update the transaction details as well.
Codecov Report
@@ Coverage Diff @@
## develop #19566 +/- ##
===========================================
- Coverage 70.29% 70.28% -0.01%
===========================================
Files 973 973
Lines 37250 37250
Branches 9603 9603
===========================================
- Hits 26183 26181 -2
- Misses 11067 11069 +2 |
Explanation
Screenshots/Screencaps
Manual Testing Steps
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Board
label.In this case, a QA Engineer approval will be be required.