chore: Fix downstream build failures introduced by #4514#4566
chore: Fix downstream build failures introduced by #4514#4566at-susie wants to merge 14 commits into
Conversation
| if (!sizeOverrides || !strokeWidthOverrides) { | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Is it expected that it will return an empty object for when either sizeOverrides or strokeWidthOverrides is undefined? Maybe this should be something like
| if (!sizeOverrides || !strokeWidthOverrides) { | |
| return result; | |
| } | |
| if (!sizeOverrides) { | |
| sizeOverrides = {}; | |
| } | |
| if (!strokeWidthOverrides) { | |
| strokeWidthOverrides = {}; | |
| } |
instead. This way, when sizeOverrides is not defined, the strokeWidthOverrides will still be applied (and vice-versa).
There was a problem hiding this comment.
That's better. Integrated your suggestion
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4566 +/- ##
==========================================
- Coverage 97.43% 97.42% -0.01%
==========================================
Files 941 941
Lines 29725 29731 +6
Branches 10803 10805 +2
==========================================
+ Hits 28962 28966 +4
- Misses 756 758 +2
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| describe('missing context fields (null guard)', () => { | ||
| it('renders without crashing when no IconProvider is present', () => { | ||
| const { container } = render(<Icon name="calendar" size="normal" />); | ||
| expect(container.querySelector('[class*="icon"]')).not.toBeNull(); |
There was a problem hiding this comment.
Can we use the Cloudscape test utils here? e.g.,
const wrapper = createWrapper(container)
wrapper.findIcon()
So that we don't depend on internals.
There was a problem hiding this comment.
Good call. Updated to use expect(wrapper(container).findIcon())
| <Icon name="calendar" size="normal" /> | ||
| </IconProvider> | ||
| ); | ||
| const iconEl = container.querySelector('[class*="icon"]') as HTMLElement; |
at-susie
requested review from
a team and
NathanZlion
and removed request for
a team
| <Icon name="calendar" size="normal" /> | ||
| </IconProvider> | ||
| ); | ||
| const iconEl = container.querySelector('[class*="icon"]') as HTMLElement; |
| }): SizeOverrideResult { | ||
| const result: SizeOverrideResult = {}; | ||
|
|
||
| if (!sizeOverrides) { |
There was a problem hiding this comment.
Why can sizeOverrides and strokeWidthOverrides be undefined in the first place if they are required in the function signature?
There was a problem hiding this comment.
Good point. Updated the parameter types to | undefined to make this explicit.
3 participants
Description
Fixes a compatibility issue that caused a downstream package build to fail after #4514 added
sizeandstrokeWidthprops toIconProvider.When the
IconProvidercontext is not present,sizeOverridesandstrokeWidthOverridescan be undefined, causingcomputeSizeOverridesto crash with TypeError: Cannot read properties of undefined. To fix this, I added a null guard that returns early when either map is undefined.Related links, issue #, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.