-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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: icon to icons for IconButton and Header component #15647
Conversation
/testenv up |
@pkdotson Ephemeral environment spinning up at http://54.212.110.82:8080. Credentials are |
Codecov Report
@@ Coverage Diff @@
## master #15647 +/- ##
==========================================
- Coverage 76.99% 76.70% -0.29%
==========================================
Files 978 984 +6
Lines 51486 51715 +229
Branches 6950 6977 +27
==========================================
+ Hits 39642 39670 +28
- Misses 11620 11821 +201
Partials 224 224
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@@ -41,6 +41,15 @@ const StyledImage = styled.div` | |||
height: ${({ theme }) => theme.gridUnit * 18}px; | |||
margin: ${({ theme }) => theme.gridUnit * 3}px 0; | |||
|
|||
.default-db-icon { | |||
font-size: ${({ theme }) => theme.gridUnit * 9}px; |
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.
Font sizes shouldn't be set using grid units. There is theme.typography.sizes
for that.
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 like we don't have a typography size large enough for what you're going for here though. Doing math with the typography sizes sounds pretty unsavory. If we don't want to add an xxxl
size maybe grid units are actually the right fit here.
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.
Oh yah forgot about that. Maybe i'll just put down the actual font-size then.
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.
LGTM, I'm okay with ignoring the sizing semantics issue.
Looks like the e2e tests might need some tweaks to select the new icons correctly. |
@yousoph are okay with the default images for the aria update? |
Thumbs up on the db icon 👍 |
Ephemeral environment shutdown and build artifacts deleted. |
…5647) * initial commit * fix test * last one * fix cypress * remove gridunit for fonsize * fix cypress * more data-test removal * last one * more data-test
…5647) * initial commit * fix test * last one * fix cypress * remove gridunit for fonsize * fix cypress * more data-test removal * last one * more data-test
…5647) * initial commit * fix test * last one * fix cypress * remove gridunit for fonsize * fix cypress * more data-test removal * last one * more data-test
SUMMARY
This pr refactors the iconbutton component default db icon to the new ant-d dbicon outlined.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
before
![PostgreSQL](https://user-images.githubusercontent.com/17326228/125369366-f0092a80-e330-11eb-9974-34ae313e5de8.png)
after
![PostgreSQL](https://user-images.githubusercontent.com/17326228/125369378-f4cdde80-e330-11eb-981e-312c560aca85.png)
TESTING INSTRUCTIONS
Test out in databasemodal and dashboard (edit view)
ADDITIONAL INFORMATION