Skip to content
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: introduce o3-grid-column-width CSS Custom Property #1776

Merged
merged 9 commits into from
Aug 27, 2024

Conversation

frshwtr
Copy link
Contributor

@frshwtr frshwtr commented Aug 13, 2024

Describe your changes

Some components outside of grids may use grid dimensions. For example, form elements may use max-width constrained by a number of columns in grid, without being part of one.

This PR adds CSS custom properties at each breakpoint to give components outside of a grid some values to work with.

Issue ticket number and link

Link to Figma designs

Checklist before requesting a review

  • I have applied percy label for o-[COMPONENT] or chromatic label for o3-[COMPONENT] on my PR before merging and after review. Find more details in CONTRIBUTING.md
  • If it is a new feature, I have added thorough tests.
  • I have updated relevant docs.
  • I have updated relevant env variables in Doppler.

@@ -10,6 +11,7 @@
repeat(var(--o3-grid-columns), 1fr) 8px;
--o3-grid-area: 'bleed-left content-start . . . . . . content-end bleed-right';
--o3-grid-columns: 8;
--o3-grid-column-width: calc((740px - var(--_o3-grid-gutters) * var(--o3-grid-gap)) / var(--o3-grid-columns));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure if a hard value of 740px is going to work here. It's going to need the px value for calculation but I don't see how I can get a fluid value outside of vw 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not going to work because there's no 740px container, the grid is flexible.

We should only need to set the two Custom Property twice. Once for the fluid grid, then once when it's fixed at 1220px.

I haven't thought about it deeply but: vw is closer to what we actually mean but I we might want to be careful of scrollbars or dynamic viewports.

Alternatively, can we assume the grid is a containing block and use 100%?

--o3-grid-column-width: calc((100% - var(--_o3-grid-gutters) * var(--o3-grid-gap)) / var(--o3-grid-columns));

Copy link
Contributor

@notlee notlee Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally, maybe --o3-grid-column-width isn't actually what we want public! If our aim is to allow people to set widths of nested items, to align with grid columns, we need to account for gaps.

This is not a very fun API to set a width of 6 cols:

div {
    width: calc(((var(--o3-grid-column-width) + var(--o3-grid-gap)) * 6) - var(--o3-grid-gap))
}

Screenshot 2024-08-13 at 17 29 44

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, can we assume the grid is a containing block and use 100%?

Does this imply that any component using this var must be inside a grid? Is this going to be problematic with form elements which may not necessarily be in a grid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a very fun API to set a width of 6 cols:

div {
    width: calc(((var(--o3-grid-column-width) + var(--o3-grid-gap)) * 6) - var(--o3-grid-gap))
}

Is this example correct? We already have gap calculated as part of o3-grid-column-width.

Copy link
Contributor Author

@frshwtr frshwtr Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

div {
width: calc(((var(--o3-grid-column-width) + var(--o3-grid-gap)) * 6) - var(--o3-grid-gap))
}

This could potentially be fixed by allowing the user to set a custom property in scope to control the value of 6:

// somewhere in o-grid.
--o3-grid-span-columns: calc(((var(--_o3-grid-column-width) + var(--o3-grid-gap)) * var(--o3-grid-span-columns-count)) - var(--o3-grid-gap))

// a local file
div {
--o3-grid-span-columns-count: 6
width: var(--o3-grid-span-columns)
}

I'm not a fan of the naming here, ideally I want to differentiate between the var to give you the width value and the var to control how many columns, maybe --o3-grid-columns-to-span?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've introduced a new variable to allow this, it will need to be paired with a user-controlled var to work. Let me know what you think, I'll write docs once we've agreed an approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great idea 👍 Naming these are hard but I like your initial suggestion.

These seem quite clearly connected:

  • --o3-grid-span-columns
  • --o3-grid-span-columns-count

These seem more loosely associated by name:

  • --o3-grid-span-columns
  • --o3-grid-columns-to-span

Copy link
Contributor

@notlee notlee Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?

  • --o3-grid-columns-to-span-width
  • --o3-grid-columns-to-span-count

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted, thanks :)

@notlee notlee temporarily deployed to origami-webs-feat-intro-6lpb9c August 13, 2024 15:14 Inactive
@notlee notlee temporarily deployed to origami-webs-feat-intro-6lpb9c August 14, 2024 12:40 Inactive
@notlee notlee temporarily deployed to origami-webs-feat-intro-6lpb9c August 14, 2024 14:30 Inactive
@frshwtr frshwtr marked this pull request as ready for review August 14, 2024 15:14
@frshwtr frshwtr requested a review from a team as a code owner August 14, 2024 15:14
@notlee
Copy link
Contributor

notlee commented Aug 14, 2024

Looks good 😄 Now just to document in the README and potentially add a demo, showing a nested element aligned to the grid, to the Story?

@notlee notlee temporarily deployed to origami-webs-feat-intro-6lpb9c August 15, 2024 15:31 Inactive
@notlee notlee temporarily deployed to origami-webs-feat-intro-6lpb9c August 16, 2024 09:33 Inactive
@@ -74,6 +74,10 @@ export function O3Grid() {
{item.text}
</div>
))}
<div style={{gridColumn: `content-start / span 6`, backgroundColor: 'tomato'}}>
Span 6
<div className='nested-element'>Nested Element</div>
Copy link
Contributor

@notlee notlee Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Styles should be inline here so they are visible in Storybook's HTML tab. Otherwise there is no way to know whether nested-element is something Origami provides or not, and no way to know how it works without opening dev tools

@notlee notlee temporarily deployed to origami-webs-feat-intro-ujua8y August 27, 2024 09:33 Inactive
Copy link
Contributor

@notlee notlee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

@frshwtr frshwtr merged commit 9ff37f4 into main Aug 27, 2024
6 checks passed
@frshwtr frshwtr deleted the feat/introduce-grid-column-width-custom-property branch August 27, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants