The bug was caused by Icon component being updated to use TS to force either name or svg props to be sent while Link was not updated to reflect that reality--in initial development it worked correctly because all props to Icon were optional.
Additionally, this ticket removed setting the icon default size to 24x24 in Link as the interim Icon updates also established that in the icon itself.
4/10/24 update:
Rolled the bugfix for #240 into this PR as flagship was waiting for that as well before moving forward. Created a new beta build for flagship with the fix.
Additionally, cleaned up the custom useEffect hook for listening for fontScale changes in the Icon component and replaced with the built in react-native hook that listens for font scale changes.
Kind of tested in iOS/Web, but Storybook doesn't really allow to allow testing because it incessantly re-adds to icon.name prop thereby preventing testing of the actual scenario of sending IconProps overrides without having the icon set (manually).
[x] Tested on iOS
[x] Tested on Android
[x] Tested on Web
Provided beta build to Chris A on the flagship team to validate both with his in progress changes and so it'll be validated by app-level QA.
4/10/24 update:
Tested around in Storybook to ensure the fix for keeping the icon alignment centered worked regardless of font scaling including with maxWidth and preventScaling attributes set to the icon. Note: it does still appear visually slightly off center due to fonts themselves biasing towards the lower part of the line height, but the icon is centered using the inspector tool.
Provided an updated beta build to Chris A on the flagship team to validate both with his in progress changes and so it'll be validated by app-level QA.
PR Checklist
Code reviewer validation:
General
[x] PR is linked to ticket(s)
[x] PR has changelog label applied if it's to be included in the changelog
[x] Acceptance criteria:
All satisfied or
Documented reason for not being performed or
Split to separate ticket and ticket is linked by relevant AC(s)
[x] Above PR sections adequately filled out
[x] If any breaking changes, in accordance with the pre-1.0.0 versioning guidelines: a CU ticket has been created for the VA Mobile App detailing necessary adjustments with the package version that will be published by this ticket
Code
[x] Tests are included if appropriate (or split to separate ticket)
Description of Change
A significant bug with overriding icon properties was noted in Slack when implementing the Link component. This ticket fixes it.
The bug was caused by Icon component being updated to use TS to force either
nameorsvgprops to be sent while Link was not updated to reflect that reality--in initial development it worked correctly because all props to Icon were optional.Additionally, this ticket removed setting the icon default size to 24x24 in Link as the interim Icon updates also established that in the icon itself.
4/10/24 update: Rolled the bugfix for #240 into this PR as flagship was waiting for that as well before moving forward. Created a new beta build for flagship with the fix.
Additionally, cleaned up the custom useEffect hook for listening for fontScale changes in the Icon component and replaced with the built in
react-nativehook that listens for font scale changes.Testing Packages
Screenshots/Video
4/10/24 update:
Testing
Kind of tested in iOS/Web, but Storybook doesn't really allow to allow testing because it incessantly re-adds to icon.name prop thereby preventing testing of the actual scenario of sending IconProps overrides without having the icon set (manually).
Provided beta build to Chris A on the flagship team to validate both with his in progress changes and so it'll be validated by app-level QA.
4/10/24 update: Tested around in Storybook to ensure the fix for keeping the icon alignment centered worked regardless of font scaling including with maxWidth and preventScaling attributes set to the icon. Note: it does still appear visually slightly off center due to fonts themselves biasing towards the lower part of the line height, but the icon is centered using the inspector tool.
Provided an updated beta build to Chris A on the flagship team to validate both with his in progress changes and so it'll be validated by app-level QA.
PR Checklist
Code reviewer validation:
changeloglabel applied if it's to be included in the changelogPublish
If changes warrant a new version per the versioning guidelines and the PR is approved and ready to merge:
maininto branchmain