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.

Sign up for GitHub

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

Adapts Blankslate to render proportionally in narrow areas #3869

Merged
merged 28 commits into from
Dec 8, 2023
Merged

Adapts Blankslate to render proportionally in narrow areas #3869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
28048e9
adapts Blankslate to render proportionally in narrow areas
mperrotti Oct 26, 2023
2c46e8d
Merge branch 'main' of github.com:primer/react into mp/responsive-bla…
mperrotti Oct 26, 2023
088744f
adds color token fallbacks
mperrotti Oct 26, 2023
33ae86c
adds changeset
mperrotti Oct 26, 2023
d306147
Merge branch 'main' into mp/responsive-blankslate
mperrotti Oct 26, 2023
b9ad3dc
formats CSS with Prettier
mperrotti Oct 26, 2023
c2c7d76
rm dead css module style
mperrotti Oct 26, 2023
df91680
uses smarter primitives
mperrotti Oct 26, 2023
78c840d
tweaks type scale
mperrotti Oct 27, 2023
36ba437
improves token usage
mperrotti Oct 27, 2023
77ab644
Merge branch 'main' into mp/responsive-blankslate
mperrotti Nov 13, 2023
0acd61f
Merge branch 'mp/responsive-blankslate' of github.com:primer/react in…
mperrotti Nov 13, 2023
b7dfad8
upgrades styled-components, converts Blankslate styles back to styled…
mperrotti Nov 13, 2023
a42a511
Merge branch 'main' into mp/responsive-blankslate
mperrotti Nov 13, 2023
d0b9ddd
adds comment explaining why 34rem for min-width
mperrotti Nov 13, 2023
b1577a3
Merge branch 'main' into mp/responsive-blankslate
mperrotti Nov 14, 2023
acb0751
Merge branch 'main' into mp/responsive-blankslate
mperrotti Nov 14, 2023
6ec504d
renders an inline style block to get around styled-component 5.x limi…
mperrotti Nov 14, 2023
d1da577
Merge branch 'main' into mp/responsive-blankslate
mperrotti Nov 16, 2023
e293e76
Merge branch 'main' into mp/responsive-blankslate
mperrotti Nov 20, 2023
e649e1b
Merge branch 'main' into mp/responsive-blankslate
mperrotti Nov 21, 2023
df28cf9
Merge branch 'main' into mp/responsive-blankslate
mperrotti Nov 22, 2023
6ceffe7
Merge branch 'main' into mp/responsive-blankslate
mperrotti Nov 27, 2023
e1f34cb
Update src/Blankslate/Blankslate.tsx
mperrotti Nov 28, 2023
6a218c2
Merge branch 'main' into mp/responsive-blankslate
mperrotti Nov 28, 2023
8e2275e
Merge branch 'main' into mp/responsive-blankslate
mperrotti Dec 5, 2023
4c8046d
Merge branch 'main' into mp/responsive-blankslate
mperrotti Dec 5, 2023
d2367ab
Merge branch 'main' into mp/responsive-blankslate
mperrotti Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/many-ants-pump.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@primer/react': patch
Copy link
Member

@siddharthkp siddharthkp Oct 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good news: This is the first component in primer/react (outside of drafts) that uses css modules. ✨

To get the css into dotcom, we need to import a single css file for primer/react (similar to primer/css)

Bad news:

This is a breaking change because now we require you to include a css file for an existing component that we didn't earlier? Without that stylesheet, existing BlankSlate instances would become unstyled.

(New components get a free pass because it's a new component with a new import statement, but tricker to change existing components like this)

Probably still fine to squeeze into dotcom, but not for open source?

The ideal order for us would be to add the one primer/react css file requirement in v36. Once that's done, we can keep adding more css bundled to that file, no worries

@joshblack @langermank Is there a workaround for this that comes to mind? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think there's any way we can use BaseStyles to import CSS modules?

If this has to be a breaking change, should we hold off on this until the next major release?

Copy link
Member

@siddharthkp siddharthkp Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do you think there's any way we can use BaseStyles to import CSS modules?

We can't use BaseStyles to import CSS modules, buuuut we could use it as a temporary place to add global css for components! 😈

If this has to be a breaking change, should we hold off on this until the next major release?

For the css files, yes. But we could break this into 2 parts:

  1. one that adds styles for narrow areas but still uses styled-components (not breaking change)
  2. another that moves the styled-components to a css file that needs import (breaking change)

To release the 2nd PR (breaking), our options are:

  1. Add it to v36 (not recommended this late in the release)
  2. Don't migrate styles to css files for Blankslate right now, continue using the sx prop until the next major release (usually, we would do this)
  3. Do a quick v37 release right after v36 with this change and related primitive changes (i don't mind this at all)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one that adds styles for narrow areas but still uses styled-components (not breaking change)

This isn't going to be possible since styled-components doesn't recognize @container, and it will get stripped out.

I think doing a quick v37 release right after v36 is the best path forward. I don't think there's any way to handle these updates without introducing a breaking change.

Copy link
Member

@siddharthkp siddharthkp Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This isn't going to be possible since styled-components doesn't recognize @container, and it will get stripped out.

As far as I know, styled-components doesn't validate syntax though and strip it out. Here's an example of container queries with styled-components: https://codesandbox.io/s/zen-hill-v939m3?file=/src/App.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@siddharthkp - so your official suggestion is to add <style type="text/css">{ContainerQuery}</style> inline?

Copy link
Member

@siddharthkp siddharthkp Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

To unblock this PR: It sounds icky but Yes 😅 (hoping that dotcom doesn't have any problems with that)

Follow up would be to start collecting candidates for v37 where we can make a css file import required.

Of course, we also have the option of pausing this PR and releasing with it the next major release. But, I'm honestly don't know what the timeline for that would look like.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry if I missed this in the thread earlier, but would another option here be to use createGlobalStyle? e.g.

import { createGlobalStyle } from 'styled-components';

const ContainerQueryStyles = createGlobalStyle`
  .BlankspaceContainer {
    container: blankslate / inline-size;
  }

  @container blankslate (min-width: 34rem) {
    /* ... */
  }
`;

function Blankslate({border, children, narrow, spacious}: BlankslateProps) {
  return (
    <>
      <ContainerQueryStyles />
      <StyledBlankslate data-border={border} data-narrow={narrow} data-spacious={spacious}>
        {children}
      </StyledBlankslate>
    </>
  );
}

Was curious if this ended up working as expected. I think this has an advantage of having a shared stylesheet instead of having a <style> tag per instance of Blankslate (if it works, that is 😅)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshblack - I think using createGlobalStyle would still results in @container (max-width:600px) getting written without an open curly brace. Check out @siddharthkp 's comment above for more context.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense! Thanks @mperrotti for clarifying

---

Adapts Blankslate to render proportionally in narrow areas.

<!-- Changed components: Blankslate -->
12 changes: 6 additions & 6 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

158 changes: 115 additions & 43 deletions src/Blankslate/Blankslate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,71 +1,147 @@
import React from 'react'
import styled from 'styled-components'
import Box from '../Box'
import {Button} from '../Button'
import {get} from '../constants'
import Link from '../Link'
import {get} from '../constants'
import styled from 'styled-components'

export type BlankslateProps = React.PropsWithChildren<{
/**
* Add a border around this component
*/
border?: boolean

/**
* Constrain the maximum width of this component
*/
narrow?: boolean

/**
* Increase the padding of this component
*/
spacious?: boolean
}>

const StyledBlankslate = styled.div`
display: grid;
justify-items: center;
padding: ${get('space.5')};
container-type: inline-size;

.Blankslate {
--blankslate-outer-padding-block: var(--base-size-32);
--blankslate-outer-padding-inline: var(--base-size-32);
display: grid;
justify-items: center;
padding: var(--blankslate-outer-padding-block) var(--blankslate-outer-padding-inline);
}

.Blankslate[data-spacious='true'] {
--blankslate-outer-padding-block: var(--base-size-80);
--blankslate-outer-padding-inline: var(--base-size-40);
}

&[data-border='true'] {
border: ${get('borderWidths.1')} solid ${get('colors.border.default')};
border-radius: ${get('radii.2')};
.Blankslate[data-border='true'] {
border: var(--borderWidth-thin) solid var(--borderColor-default, ${get('colors.border.default')});
border-radius: var(--borderRadius-medium);
}

&[data-narrow='true'] {
.Blankslate[data-narrow='true'] {
margin: 0 auto;
max-width: 485px;
}

&[data-spacious='true'] {
padding: ${get('space.9')} ${get('space.6')};
.Blankslate-Heading,
.Blankslate-Description {
margin: 0;
margin-bottom: var(--stack-gap-condensed);
}

.BlankSlateAction {
margin-top: ${get('space.3')};
.Blankslate-Heading {
font-size: var(--text-title-size-medium);
font-weight: var(--text-title-weight-medium);
}

.BlankSlateAction:first-of-type {
margin-top: ${get('space.4')};
.Blankslate-Description {
color: var(--fgColor-muted, ${get('colors.fg.muted')});
font-size: var(--text-body-size-large);
}

.BlankSlateAction:last-of-type {
margin-bottom: ${get('space.2')};
.Blankslate-Action {
margin-top: var(--stack-gap-normal);
}

.Blankslate-Action:first-of-type {
margin-top: var(--stack-gap-spacious);
}

.Blankslate-Action:last-of-type {
margin-bottom: var(--stack-gap-condensed);
}
`

export type BlankslateProps = React.PropsWithChildren<{
/**
* Add a border around this component
*/
border?: boolean
const BlankslateContainerQuery = `
/* At the time these styles were written,
34rem was our "small" breakpoint width */
@container (max-width: 34rem) {
${StyledBlankslate} .Blankslate {
--blankslate-outer-padding-block: var(--base-size-20);
--blankslate-outer-padding-inline: var(--base-size-20);
}

/**
* Constrain the maximum width of this component
*/
narrow?: boolean
${StyledBlankslate} .Blankslate[data-spacious='true'] {
--blankslate-outer-padding-block: var(--base-size-44);
--blankslate-outer-padding-inline: var(--base-size-28);
}

/**
* Increase the padding of this component
*/
spacious?: boolean
}>
${StyledBlankslate} .Blankslate-Visual {
margin-bottom: var(--stack-gap-condensed);
max-width: var(--base-size-24);
}

${StyledBlankslate} .Blankslate-Visual svg {
width: 100%;
}

${StyledBlankslate} .Blankslate-Heading {
font-size: var(--text-title-size-small);
}

${StyledBlankslate} .Blankslate-Description {
font-size: var(--text-body-size-medium);
}

${StyledBlankslate} .Blankslate-Action {
margin-top: var(--stack-gap-condensed);
}

${StyledBlankslate} .Blankslate-Action:first-of-type {
margin-top: var(--stack-gap-normal);
}

${StyledBlankslate} .Blankslate-Action:last-of-type {
margin-bottom: calc(var(--stack-gap-condensed) / 2);
}
`

function Blankslate({border, children, narrow, spacious}: BlankslateProps) {
return (
<StyledBlankslate data-border={border} data-narrow={narrow} data-spacious={spacious}>
{children}
</StyledBlankslate>
<>
{/*
This is a workaround so we can use `@container` without upgrading `styled-components` to 6.x
See [this comment](https://github.com/primer/react/pull/3869#discussion_r1392523030) for more info
*/}
<style type="text/css">{BlankslateContainerQuery}</style>
<StyledBlankslate>
<div className="Blankslate" data-border={border} data-narrow={narrow} data-spacious={spacious}>
{children}
</div>
</StyledBlankslate>
</>
)
}

export type VisualProps = React.PropsWithChildren

function Visual({children}: VisualProps) {
return <Box sx={{color: 'fg.muted', mb: 3}}>{children}</Box>
return <span className="Blankslate-Visual">{children}</span>
}

export type HeadingProps = React.PropsWithChildren<{
Expand All @@ -74,7 +150,7 @@ export type HeadingProps = React.PropsWithChildren<{

function Heading({as = 'h2', children}: HeadingProps) {
return (
<Box as={as} sx={{m: 0, mb: 2}}>
<Box as={as} className="Blankslate-Heading">
{children}
</Box>
)
Expand All @@ -83,11 +159,7 @@ function Heading({as = 'h2', children}: HeadingProps) {
export type DescriptionProps = React.PropsWithChildren

function Description({children}: DescriptionProps) {
return (
<Box as="p" sx={{color: 'fg.muted', m: 0, mb: 2}}>
{children}
</Box>
)
return <p className="Blankslate-Description">{children}</p>
}

export type PrimaryActionProps = React.PropsWithChildren<{
Expand All @@ -96,7 +168,7 @@ export type PrimaryActionProps = React.PropsWithChildren<{

function PrimaryAction({children, href}: PrimaryActionProps) {
return (
<div className="BlankSlateAction">
<div className="Blankslate-Action">
<Button as="a" href={href} variant="primary">
{children}
</Button>
Expand All @@ -110,7 +182,7 @@ export type SecondaryActionProps = React.PropsWithChildren<{

function SecondaryAction({children, href}: SecondaryActionProps) {
return (
<div className="BlankSlateAction">
<div className="Blankslate-Action">
<Link href={href}>{children}</Link>
</div>
)
Expand Down
Loading