Create psammead grid wrappers #6618
Conversation
ghost
added
ws-articles
ghost
self-assigned this
|
I know it seems there are a lot of changes in this PR. In reality the changes are very few. This are the components to review.
|
ghost
marked this pull request as ready for review
| @@ -63,7 +60,7 @@ const ArticleTimestamp = ({ | |||
| prefix: articleTimestampPrefix, | |||
| }; | |||
|
|
|||
| const Wrapper = popOut ? PopOutGridItemMedium : GridItemConstrainedMedium; | |||
| const Wrapper = popOut ? PopOutGridItemMedium : GridItem; | |||
There was a problem hiding this comment.
I was trying to understand why this had changed from GridItemConstrainedMedium to GridItem and I came across GridItemMedium.
What is the difference between GridItem and GridItemMedium? They appear to be using a very similar configuration.
There was a problem hiding this comment.
The difference was that GridItem applied margins and GridItemMedium had no margins. I have removed GridItemMedium and added enableMargins prop on GridItem
| const GridConstraints = { | ||
| headline: GridItemConstrainedLarge, | ||
| headline: GridItemLarge, | ||
| subheadline: GridItemConstrainedMedium, | ||
| }; |
There was a problem hiding this comment.
Please can you help me understand why we have changed GridItemConstrainedLarge to GridItemLarge, but not GridItemConstrainedMedium to GridItemMedium?
There was a problem hiding this comment.
I was trying to minimize the changes needed for the Grid updates to work. The next steps would be to gradually replace the old implementations in small PR's to aid faster review.
src/app/lib/styledGrid/index.jsx
Outdated
| group5: 10, | ||
| }; | ||
|
|
||
| export const MediumGridWrapper = ({ children }) => { |
There was a problem hiding this comment.
Can I suggest we follow the same naming convention to other grid components and name the size as a suffix rather than a prefix? E.G. MediumGridWrapper to GridWrapperMedium.
| @@ -27,6 +29,160 @@ import { | |||
| gridContainerLargeCss, | |||
| } from '../layoutGrid'; | |||
|
|
|||
| export const MediumContentContainer = styled.div` | |||
There was a problem hiding this comment.
I am confused by the name of this component. Please can you help me understand how it differs from MediumGridWrapper?
There was a problem hiding this comment.
MediumGridWrapper now renamed to GridWrapperMedium acts as a parent grid wrapper to its children. MediumContentContainer doesn't. MediumContentContainer is meant to act as a wrapper for pages like the PhotoGallery, ArticlePage and MAP which have their contents centered on the page.
There was a problem hiding this comment.
I'm a bit confused on the naming conventions as well, I don't think MediumContentContainer is a good fit. Maybe something tied in with it being a Single column container and then maybe a comment on where the container is used to make it more clear.
src/app/lib/styledGrid/index.jsx
Outdated
| children: node.isRequired, | ||
| }; | ||
|
|
||
| export const GridItem = ({ children }) => { |
There was a problem hiding this comment.
GridItem seems very similar to GridItemMedium other than the margin configurations. It would be nice to see on the PR examples of GridItem's use so I can better understand where they are intended to be used.
There was a problem hiding this comment.
They are similar. I guess this could be made to one function and pass in a prop like enableMargin
There was a problem hiding this comment.
Done here 6af6c48. GridItem is used on the Paragraph and Image container.
…h into create-psammead-grid-wrappers
…h into create-psammead-grid-wrappers
Part of #3686
Overall change:
Create
Gridcontainers based onpsammead-gridand use them to replaceGridimplementation found in simorgh.Code changes:
MediumContentContainerto be used as a content wrapper that centres the contents ofArticlePage,PhotoGalleryPage,MediaAssetPage.GridItemMediumto match the properties ofGridItemConstrainedMedium.GridItemSmallto match the properties ofGridItemConstrainedSmall.GridItemLargeto match the properties ofGridItemConstrainedLargeNoMargin.Testing:
CYPRESS_APP_ENV=local CYPRESS_SMOKE=false npm run test:e2e:interactive)