Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Control height of Brand SVG #480

Merged
merged 40 commits into from
May 20, 2019
Merged

Control height of Brand SVG #480

merged 40 commits into from
May 20, 2019

Conversation

andrew-nowak
Copy link
Contributor

@andrew-nowak andrew-nowak commented Apr 29, 2019

Resolves #466
Resolves #499

Overall change: Height of the SVG in the Brand component is dynamically scaled between the prop values minWidth and maxWidth. Also the Banner height and padding changes at 600px.

Code changes:

  • Banner height is now 56px when window is 0-599px and 80px for 600+px
  • SVG left-right padding is now 8px when window is 0-599px and 16px for 600+px
  • SVG dynamically scales (while remaining centred) between the minWidth and maxWidth values.
  • SVG is vertically centred in the banner using margin-top
  • Storybook: add knobs to Brand story which allow for passing of minWidth maxWidth and height to showcase the flexibility of the component and the dynamic scaling of the SVG

Testing 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.


  • I have assigned myself to this PR and the corresponding issues
  • Tests added for new features
  • Test engineer approval

@andrew-nowak andrew-nowak self-assigned this Apr 29, 2019
@andrew-nowak andrew-nowak added the ws-home Tasks for the WS Home Team label Apr 29, 2019
@andrew-nowak andrew-nowak marked this pull request as ready for review April 29, 2019 13:31
Copy link
Contributor

@dr3 dr3 left a 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/README.md Outdated Show resolved Hide resolved
packages/components/psammead-brand/README.md Outdated Show resolved Hide resolved
packages/components/psammead-brand/README.md Outdated Show resolved Hide resolved
dr3
dr3 previously approved these changes Apr 29, 2019
Copy link
Contributor

@dr3 dr3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!
lgtm

Copy link

@ghost ghost left a 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.

packages/components/psammead-brand/CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: aacn500 <10963046+aacn500@users.noreply.github.com>
ghost
ghost previously approved these changes Apr 30, 2019
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@greenc05
Copy link
Contributor

Has this had a developer accessibility swarm carried out on it?

@pjlee11
Copy link
Contributor

pjlee11 commented May 16, 2019

@greenc05 good point, this PR pre-dated the developer accessibility swarm workflow so will need one.

@pjlee11 pjlee11 dismissed stale reviews from 12 and DenisHdz via e8f54e8 May 17, 2019 13:05
@pjlee11
Copy link
Contributor

pjlee11 commented May 17, 2019

Developer accessibility swarm complete!

@ghost ghost mentioned this pull request May 17, 2019
3 tasks
@david-boydell
Copy link

Still looks good, RFM.

@pjlee11 pjlee11 merged commit 7ac8ae7 into latest May 20, 2019
@pjlee11 pjlee11 deleted the brand-pass-svg-height branch May 20, 2019 09:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ws-home Tasks for the WS Home Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make brand SVG dynamically sized per service Allow for brand to be passed SVG height
10 participants