This repository has been archived by the owner on Dec 6, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 12
Support responsive column widths #726
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
29107ab
support responsive column widths
SiAdcock cfd3986
supporting hiding above and below breakpoints
SiAdcock 83828e4
Merge branch 'main' into sa-responsive-column-width
SiAdcock 3ff8beb
handle empty column width array
SiAdcock 079cb9a
Merge branch 'sa-responsive-column-width' of github.com:guardian/sour…
SiAdcock 09cda83
Merge branch 'main' into sa-responsive-column-width
SiAdcock b597775
Merge branch 'main' into sa-responsive-column-width
SiAdcock File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import React from 'react'; | ||
import { Columns, Column, Container } from '../../index'; | ||
import { sport } from '@guardian/src-foundations/palette'; | ||
import { css } from '@emotion/react'; | ||
|
||
const contents = css` | ||
text-align: center; | ||
background-color: ${sport[600]}; | ||
`; | ||
|
||
export const responsive = () => ( | ||
<Container> | ||
<Columns> | ||
<Column width={[1 / 2, 1 / 4]}> | ||
<div css={contents}>50% at mobile, 25% above tablet</div> | ||
</Column> | ||
<Column width={[1 / 2, 3 / 4]}> | ||
<div css={contents}>50% width at mobile, 75% above tablet</div> | ||
</Column> | ||
</Columns> | ||
</Container> | ||
); | ||
|
||
responsive.story = { | ||
name: 'responsive columns', | ||
parameters: { | ||
viewport: { defaultViewport: 'tablet' }, | ||
}, | ||
}; | ||
|
||
export const responsivelyHide = () => ( | ||
<Container> | ||
<Columns> | ||
<Column width={[0, 1 / 4]}> | ||
<div css={contents}> | ||
Not visible at mobile, 25% above tablet | ||
</div> | ||
</Column> | ||
<Column width={[1, 3 / 4]}> | ||
<div css={contents}>100% at mobile, 75% above tablet</div> | ||
</Column> | ||
</Columns> | ||
</Container> | ||
); | ||
|
||
responsivelyHide.story = { | ||
name: 'responsively hide columns', | ||
parameters: { | ||
viewport: { defaultViewport: 'tablet' }, | ||
}, | ||
}; |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
if
width
no longer has a default value of0
, is this a breaking change?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.
If you were relying on the default before then I think you still get the same behaviour (due to these changes) but if you're explicitly passing 0 you will now get different behaviour so maybe it's a breaking change? Also, if you pass an empty array I don't think it's applying any styling. Maybe that should be the same as not passing a width?
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.
@jamie-lynch is correct, if you were explicitly passing
width={0}
before, the behaviour would have been the same as if you didn't pass a width (i.e. the column's width would be the remainder of container width shared between all zero-width columns). This behaviour was not documented. Arguably the behaviour is bizarre and unexpected, even a bug.The new behaviour is to hide a column that has a 0 width. I think this is more intuitive.
Although this is technically a breaking change, I'm inclined not to treat it as such from a semver / release perspective. Nobody is currently relying on this behaviour. Let's close this loophole before people start relying on it.
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.
Good catch with the empty array 🙏
I've updated the logic to treat an empty array the same as passing not passing a width. I hope that makes sense!