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

Brand should have max width of 80rem #649

Merged
merged 8 commits into from
Jun 19, 2019

Conversation

Bopchy
Copy link
Contributor

@Bopchy Bopchy commented Jun 18, 2019

Resolves #645

Overall change: Update brand to have a max-width of 80rem.

Code changes:

  • Update brand to have max-width of 80rem.
  • Update snapshots.

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

@Bopchy Bopchy self-assigned this Jun 18, 2019
@Bopchy Bopchy added ws-fp-v1 ws-home Tasks for the WS Home Team labels Jun 18, 2019
@Bopchy Bopchy marked this pull request as ready for review June 18, 2019 15:28
@@ -19,6 +22,11 @@ const PADDING_AROUND_SVG_BELOW_600PX = 32;
const conditionallyRenderHeight = (svgHeight, padding) =>
svgHeight ? `height: ${(svgHeight + padding) / 16}rem` : '';

const SvgWrapper = styled.div`
max-width: ${GEL_GROUP_4_SCREEN_WIDTH_MAX};
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though they are the same value 🙈 Can you use GEL_GROUP_5_SCREEN_WIDTH_MIN here instead please :P Its what we used before :)

@@ -100,18 +108,20 @@ const StyledBrand = ({
<Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this doesnt work when the svg is a link weirdly :P May need to move the wrapper up a bit

@Bopchy Bopchy requested a review from dr3 June 19, 2019 08:55
Copy link
Contributor

@j-pendlebury j-pendlebury left a comment

Choose a reason for hiding this comment

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

needs a package-lock update, but otherwise LGTM 👍

@david-boydell david-boydell self-assigned this Jun 19, 2019
@david-boydell
Copy link

Looks good, this is ready for merge!

@Bopchy Bopchy merged commit a6c6cce into latest Jun 19, 2019
@jamesbhobbs jamesbhobbs deleted the 645-brand-should-have-max-width-80rem branch June 19, 2019 13:26
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.

Brand should have max width 80rem
4 participants