-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Onward Content Design Refactor #9691
Conversation
@@ -405,8 +410,10 @@ export const Card = ({ | |||
return ( | |||
<CardWrapper | |||
format={format} | |||
showTopBar={!isOnwardContent} |
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.
Have passed this rather than just using isOnwardContent
in the CardWrapper as it is more abstracted and reusable in future
Size Change: -13 kB (-2%) Total Size: 733 kB
ℹ️ View Unchanged
|
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.
Looks great, I'm unsure about the prop abstraction but its not a blocker
…ndary, depending if its a Front carousel or an article carousel
9be27c2
to
5d69bd7
Compare
I think it we're going to refactor cards we should consider moving to CSS grid which would drastically simplify their CSS and DOM structure. Happy to demo some stuff |
I'm not opposed to using grid but I don't really think this PR is the place for it - I've mainly made use of the existing card with some minor tweaks (top bar showing, rounded corners and and different background). Think it looks like I've made more changes than I have because i've un-nested some css! |
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.
I'm not opposed to using grid but I don't really think this PR is the place for it
Fair point – I disabled whitespace change and the diff was much smaller. One for another time 👍
…of the card for onwards content
Seen on PROD (merged by @sophie-macmillan 9 minutes and 9 seconds ago) Please check your changes! |
What does this change?
This updates the design for all the onwards contents carousels. The front carousel should not be updated so we use the
isOnwardsContent
prop to distinguish between the two.I didn't think the changes required were significant to create a new carousel or card, but if whoever reviews this feels differently we can discuss and split it out. The card is quite big and has a fair amount of logic in it already.
Why?
Part of a redesign
Resolves #7390
Screenshots