-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
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.
Really good work, the centering logic works really well, very nice adjusting it in storybook and seeing it scale nicely :D Just a couple tweaks
packages/components/psammead-brand/src/__snapshots__/index.test.jsx.snap
Outdated
Show resolved
Hide resolved
stops React rendering the heights object
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.
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.
Just one tiny change.
Co-Authored-By: aacn500 <10963046+aacn500@users.noreply.github.com>
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 👍
Has this had a developer accessibility swarm carried out on it? |
@greenc05 good point, this PR pre-dated the developer accessibility swarm workflow so will need one. |
Developer accessibility swarm complete! |
Still looks good, RFM. |
Resolves #466
Resolves #499
Overall change: Height of the SVG in the Brand component is dynamically scaled between the prop values
minWidth
andmaxWidth
. Also the Banner height and padding changes at 600px.Code changes:
minWidth
andmaxWidth
values.margin-top
minWidth
maxWidth
andheight
to showcase the flexibility of the component and the dynamic scaling of the SVGTesting notes
Check the brand component in storybook and test cross browser the functionality to dynamically scale the SVG's at small widths works as expected.