-
Notifications
You must be signed in to change notification settings - Fork 11
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
refactor(buttons): convert buttons in scans and credentials list views #142
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #142 +/- ##
======================================
Coverage ? 75.97%
======================================
Files ? 101
Lines ? 3292
Branches ? 920
======================================
Hits ? 2501
Misses ? 701
Partials ? 90 Continue to review full report at Codecov.
|
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.
Double check with Ben on the button but otherwise, lgtm!
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.
Yeah, not sure why the merge reports is/was a secondary button. It's the only real action on the page. Let's make it the primary button.
e17d910
to
60f30d5
Compare
60f30d5
to
b5c4c91
Compare
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.
Thank you for your reviews and feedback @cdcabrera @bclarhk !
I've switched the button to use the primary
variant, and have uploaded a screenshot with the updated appearance of the "merge reports" button.
What's included
...
Converts PF3 buttons to PF4 buttons in list view of Scans and Credentials screens.
This includes the icons on the right of every list item, and the "Merge Reports" button on the Scans screen.
Notes
The changes in this PR are aligned with the code and feedback in #136
Example
...
Before
After
Updates issue/story
DISCOVERY-149
#134