Add MoreGalleries component with a story - #14538
Conversation
85206ec to
3755b1f
Compare
| trailTextColour?: string; | ||
| /** Controls visibility of trail text on various breakpoints */ | ||
| hideUntil?: 'tablet' | 'desktop'; | ||
| hideUntil?: 'tablet' | 'desktop' | 'never'; |
There was a problem hiding this comment.
Why did I need a never value?
The hideUntil can be undefined. When it's undefined, the component will always add the trail text, but in css display is set to none for viewport below tablet.
When hideUntil is not undefined, the trail text will be wrapped within a Hide element and become hidden until the viewport that's given by the hideUntil. However, the css display: none; is also set for until.tablet in this case.
So no matter what, we always have display: none; on trail text for viewport below tablet. But in our case for more-galleries onward container, we want to always display trail text within the flexSplash card for all viewports.
That's why I added the never value in order to always display the trail for all viewports.
@arelra and @abeddow91 what do you think about this change?
There was a problem hiding this comment.
I wonder if it might be simpler to introduce an optional prop such as:
hide: boolean which defaults to true
This then controls whether the trail text applies the <Hide> component or not.
In existing usages of TrailText this will default to true and apply <Hide> but for the galleries onwards container we could pass hide=false which would mean the hiding behaviour would not apply ?
There was a problem hiding this comment.
Great idea, I instead called the new param as hideOnSmallBreakpoints to make it clearer what it's doing, is that OK?
There was a problem hiding this comment.
Having thought about this a little more I'm not sure I understand why we don't use the absence or presence of hideUntil to control whether hiding is activated?
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
Co-authored-by: Jamie B <53781962+JamieB-gu@users.noreply.github.com>
3755b1f to
367a603
Compare
DanielCliftonGuardian
left a comment
There was a problem hiding this comment.
This looks good. Just one suggestion.
Instead always pass hideUntil when hiding is required and pass undefined when hiding is not required (trail text needs to be visible for all breakpoints) Co-authored-by: Ravi <7014230+arelra@users.noreply.github.com>
JamieB-gu
left a comment
There was a problem hiding this comment.
Looks good! Some questions and suggestions.
| uniqueId?: string; | ||
| /** The Splash card in a flexible container gets a different visual treatment to other cards */ | ||
| /** The Splash card in a flexible or onward container gets a different visual treatment to other cards */ | ||
| isFlexSplash?: boolean; |
There was a problem hiding this comment.
I wonder if we should rename this field or use a seperate one here. I'm not sure we should be coupling a flexible splash card with an onwards container splash, unless they are truely the same card. If they are then perhaps renaming to something more generic (ie removing the flexible part would be preferable).
Does the onwards "splash" card share the same characteristics as a flexible splash card?
There was a problem hiding this comment.
Thanks for pointing this out @abeddow91
You are right, it seems the characteristics of a flexible splash card is different from an onward splash card.
The isFlexSplash is primarily used to deal with positioning the sub links. But what's important in an onward splash card is that it needs to have the trail text while the non-splash cards do not have trail text.
I created a new prop isOnwardSplash for this in here 73cd910
|
Overdue on PROD (merged by @marjisound 47 minutes and 22 seconds ago) What's gone wrong? |
|
Seen on PROD (merged by @marjisound 1 hour, 19 minutes and 8 seconds ago) Please check your changes! |
Paired with @JamieB-gu
What does this change?
This PR creates a new MoreGalleries component which will be used for the more galleries onward content section on gallery articles. For this component we were given new design that is different from how this section looks currently in production.
This new component is not currently used. The usage will be implemented in an upcoming PR.
Screenshots
This PR fixes #14310