fix: Support icons for button dropdown groups#3710
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3710 +/- ##
========================================
Coverage 97.00% 97.00%
========================================
Files 821 821
Lines 23856 23862 +6
Branches 8377 7953 -424
========================================
+ Hits 23141 23147 +6
- Misses 666 709 +43
+ Partials 49 6 -43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
avinashbot
left a comment
avinashbot
left a comment
There was a problem hiding this comment.
Also a general comment to update the PR title to be more friendly - they show up in the release notes!
| {item.text && ( | ||
| <p className={clsx(styles.header, { [styles.disabled]: disabled })} aria-hidden="true"> | ||
| {item.text} | ||
| <span style={{ display: 'flex', alignItems: 'center' }}> |
There was a problem hiding this comment.
Inline styles here - best to avoid them unless necessary for things like dynamic calculations. Inline styles don't match our Content-Security-Policy recommendations.
There was a problem hiding this comment.
Also, out of curiosity, since you have align-items: center on the .header anyway, why have another wrapper span? (Maybe there are good reasons, just curious)
There was a problem hiding this comment.
Good point! I used it during the troubleshooting after the rollback when align-items: center wasn't in the header yet, but it might not be necessary any more. I'm going to test it and remove it if possible.
| margin-block-end: -1px; | ||
| } | ||
|
|
||
| .icon-wrapper { |
There was a problem hiding this comment.
If the parent is going to be a display: flex, I think you could just put gap: awsui.$space-xxs on the parent and not have to deal with a wrapper.
There was a problem hiding this comment.
that was the same idea that I had as well, but it caused the caret to be misplaced in button-dropdown/disabled-reason. So I circumvented that issue with the wrapper
mariksav
changed the title
|
I removed the span wrapper and everything works fine. |
mariksav
force-pushed
the
fix/icon-for-button-dropdown
branch
from
6013ac2 to
763c2fb
Compare
|
I retested the change and saw that the span wrapper was actually necessary, because otherwise the website formats in an unpleasant way in a few cases. So I will need to readd it, but I removed the inline styling. Please let me know what you think |
avinashbot
left a comment
avinashbot
left a comment
There was a problem hiding this comment.
Okay, a few extra divs/spans is no big deal - not going to block a merge just to spend time diving into a weird flex situation lol
2 participants
Description
Fix for #3684
Customers want to assign icons to expandable ButtonDropdowns, which wasn't possible so far. In order to enable that functionality, the rendering for icons for expandable Dropdown Buttons was added and test cases implemented. Furthermore, the style was changed to keep the look of the page consistent.
Issue: AWSUI-60667
How has this been tested?
Tested locally with unit and integration tests.
Ran through own pipeline.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.