-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: Add Breakout component that lets media content stick out of a Spotlight #1397
Merged
Conversation
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
VincentSmedinga
commented
Jul 15, 2024
VincentSmedinga
changed the title
Feature/des 802 stick out grid
Feat: Use CSS Grid features to let content stick out of a Spotlight
Jul 15, 2024
VincentSmedinga
changed the title
Feat: Use CSS Grid features to let content stick out of a Spotlight
feat: Use CSS Grid features to let content stick out of a Spotlight
Jul 15, 2024
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
July 15, 2024 18:54
Destroyed
dlnr
reviewed
Jul 16, 2024
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.
This makes me wonder if grid should be a design system component instead of some guidelines. It's getting overcomplicated and solving this in application specific CSS is going to be a lot lighter/faster.
Just a couple of remarks, not a real review.
VincentSmedinga
force-pushed
the
feature/DES-802-stick-out-grid
branch
from
September 9, 2024 09:40
2628bba
to
ab7a701
Compare
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
September 9, 2024 09:40
Destroyed
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
September 11, 2024 12:01
Destroyed
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
September 11, 2024 12:09
Destroyed
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
September 11, 2024 15:21
Destroyed
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
September 19, 2024 15:05
Destroyed
alimpens
reviewed
Sep 20, 2024
Co-authored-by: Aram <[email protected]>
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
September 24, 2024 12:33
Destroyed
alimpens
reviewed
Sep 25, 2024
Co-authored-by: Aram <[email protected]>
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
September 25, 2024 10:32
Destroyed
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
September 25, 2024 10:35
Destroyed
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
September 25, 2024 10:37
Destroyed
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
September 25, 2024 11:29
Destroyed
We’ll solve this soon in both CSS and TypeScript
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
September 25, 2024 11:45
Destroyed
We’ll solve this soon in both CSS and TypeScript
# Conflicts: # packages/css/src/components/index.scss # packages/react/src/index.ts
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
September 25, 2024 13:19
Destroyed
github-actions
bot
temporarily deployed
to
demo-DES-802-stick-out-grid
September 25, 2024 13:24
Destroyed
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Update (2024/09/13): We decided against making Grid more complex (for now). We copied Grid, including the new row spanning and gap covering features, to a separate Breakout component, then reverted the changes in Grid, maintaining the approach outlined below.
tl;dr:
I had this lingering idea that the feature set of CSS Grid should be rich enough to meet (most of) our ‘Spotlight with break-out content challenge’. We already use stacking grid cells in
Overlap
, like Niels I thought grid rows should play a role, and we need some box alignment features.With our current approach of a Grid in a Spotlight, we could never let something on the grid pop out of the Spotlight, as it contains the Grid and would grow with it.
So I turned it upside down and put the Spotlight in a Grid. The idea was to have a grid of two columns and two rows, with the Spotlight in the bottom two cells and the image in the two at the right, using overlapping grid cells as mentioned. The realisation then was that it would obfuscate half of the text.
My solution was to separate the Spotlight from the text. It may be a bit controversial, but there’s no way both will become misaligned and the text unreadable because they use the bottom left cell. The empty Spotlight is just a coloured div, and the component API still ensures the correct colours are used. It does need a negative margin to overlap the horizontal and vertical whitespace, but we can reuse the design tokens here and use the
padding
props to make it work with content above and below.The rows' height is perfect by default—their
auto
size makes the Spotlight exactly as high as the text, even while they are separated in the DOM, and the image takes the rest of the height.I’ve extended the existing API with rows. This makes it easy to stack the parts vertically instead of horizontally on narrow screens.
The approach works with any of the aspect ratios, any content even. It is a bit of a puzzle to make this example look good on various widhts – text an images scale totally differently. I had to give up the 50/50 layout on the 8-column grid; check out the commit before ‘Stack image and text vertically in medium grid as well’ to see why. And I wrapped all Spotlight stories in a Screen because the sticking out broke spectacularly on my ultra-wide monitor. (Which might be a good idea for all stories; we can combine it with the language attribute.)
The increased size of the CSS for the Grid component is a downside. We should shorten these class names. An idea is to make them utility classes with abbreviations like we do for ‘margin bottom’. As a bonus, we can put this class name directly on the paragraph or image without needing an intermediary Grid Cell div.
There are still a few things funky in this approach, see my own comments, but I think the aspects that work well – row height, grid placement, responsiveness, general applicability, much automatic stuff – outweigh them. There’s a few more things we could hide into an API.
The example is called ‘Stick out’ because ‘Break out’ seems to be used for sections stretching to the window's edge while other content gets horizontally constrained. ‘Pop out’ could be an alternative, too. I’ve checked with Thomas, and he’s on the same track :)