feat(806) container queries and storybook #877

LukeFinch · 2023-05-05T09:03:09Z

closes #806

What

Background - why this is needed
Allows users to add custom media queries and container queries into props/overrides
What did you do

Added new CSSQuery type
Added new ResponsiveValue type - a union of CSSQuery and MQ
Changed MQ to ResponsiveValue for some components (grid layout, block, textblock)
What does the reviewers should expect
You can use one of the above components, and add a "rules" object to the style override.

      <GridLayout
        rows={{
          rules: [
            {
              rule: '@container grid-container (width <= 200px)',
              value: '1fr 1fr',
            },
            {
              rule: '@container grid-container (width > 200px)',
              value: '1fr 1fr 1fr 1fr',
            },
          ],
        }}
        columns={{
          xs: '1fr',
          md: '2fr',
          rules: [
            {
              rule: '@container grid-container (width <= 200px)',
              value: '1fr 1fr 1fr 1fr 1fr 1fr 1fr 1fr',
            },
            {
              rule: '@container grid-container (width > 200px)',
              value: '1fr 1fr',
            },
          ],
        }}
      >

This adds a new style to the page with emotion that handles the container query

I have done:

Written unit tests against changes
Written functional tests against the component and/or NewsKit site
Updated relevant documentation

I have tested manually:

The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
The screen reader reads and flows through the elements as expected.
There are no new errors in the browser console coming from this PR.
When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

pp-serviceaccount · 2023-05-05T10:42:25Z

You can preview these changes on:
Documentation Site: http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/806-container-queries-storybook
Storybook: http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/806-container-queries-storybook/storybook

src/block/__tests__/block.stories.tsx

jps · 2023-05-09T10:17:00Z

package.json

-    "@emotion/react": "^11.1.5",
-    "@emotion/styled": "^11.10.4",
+    "@emotion/react": "^11.10.5",
+    "@emotion/styled": "^11.10.5",


Will these changes no longer work with lower versions of emotion? Do note that we have emotion versions specified in the peerDependencies for consumers and this will likely need to be upped.

It's a bit grey to me if this would actually make this a breaking change or not. Given it's only a minor version bump on a dependency we could argue it isn't... but I guess there is still some level of risk for consumers who are upgrading.

It didn't work on the version we had, and required the bump. I'm not sure which specific minimum version it's in. I could narrow that down. But ultimately, it does require moving to a higher emotion version to use container queries

11.10.4 will be the lowest supported version can we please update the peer dependencies?

also I think it should be sub 12 not the thin range that it's currently specified to

src/block/__tests__/block.stories.tsx

src/block/block.tsx

jps · 2023-05-09T11:02:02Z

src/grid-layout/types.ts

  } & LogicalProps;
-} & Omit<React.HTMLAttributes<HTMLDivElement>, 'children'>;
+} & ContainerQueryProps &


should this live at the same level as the logical props? e.g. in the overrides?

Addressed in the latest commit

jps · 2023-05-09T11:05:56Z

src/utils/style/types.ts

@@ -7,3 +7,19 @@ export type MQPartial<T> = Partial<{
 }>;

 export type MQ<T> = T | MQPartial<T>;
+
+export type ResponsiveValue<T> = MQ<T> | CSSQueryRules<T>;


Can we add a description to these news types, it should make it easier for consumers to understand what they are looking at.

We need to update the docs site as well so that reflects these changes.

src/grid-layout/__tests__/stories/grid-layout.stories.tsx

mstuartf · 2023-05-10T10:18:57Z

👏 This is cool

mutebg · 2023-05-11T08:03:30Z

src/block/__tests__/block.stories.tsx

+          overrides={{
+            containerName: 'container-small',
+            containerType: 'inline-size',
+          }}


Block does not have overrides, these need to be props. I think the same apply to the other componnets

Originally I had them as props, @jps suggested moving them into overrides - to be the same level as logical props
Ideally would keep them consistent across all components

Well, the thing is that Block does not have overrides, so the logical props on the Block are props, <Block marginInline />
GridLayout has overrides (which was wrong) and in that case they can be part of the overrides

src/grid-layout/__tests__/stories/grid-layout.stories.tsx

src/card-composable/__tests__/__stories/card-composable.stories.tsx

mutebg · 2023-05-11T08:14:10Z

src/grid-layout/types.ts

@@ -11,7 +11,8 @@ export type GridLayoutItemProps = BlockProps & {
  alignSelf?: MQ<string>;
  column?: MQ<string>;
  row?: MQ<string>;
-} & React.HTMLAttributes<HTMLElement>;
+} & ContainerQueryProps &


The Block already have these as "overrides' which I mentioned need to be just props.
And this is adding it second time,

mutebg · 2023-05-11T08:15:19Z

src/grid-layout/types.ts

+    minHeight?: ResponsiveValue<string>;
+    maxHeight?: ResponsiveValue<string>;
+  } & LogicalProps &
+    ContainerQueryProps;


same as block -> move to props

src/utils/responsive-helpers.ts

mutebg · 2023-05-11T08:23:13Z

src/utils/style/types.ts

@@ -7,3 +7,19 @@ export type MQPartial<T> = Partial<{
 }>;

 export type MQ<T> = T | MQPartial<T>;
+
+export type ResponsiveValue<T> = MQ<T> | CSSQueryRules<T>;


We need to update the docs site as well so that reflects these changes.

mutebg · 2023-05-11T08:23:37Z

src/utils/style/types.ts

+  @param value - CSS value or token
+*/
+export type CSSQuery<T> = {
+  rule: `@media ${string}` | `@container ${string}`;


that's very clever ;)

src/utils/style/utils.ts

jps · 2023-05-16T14:59:07Z

src/utils/logical-properties.ts

-  marginBlockStart?: MQ<string>;
-  marginBlockEnd?: MQ<string>;
-  marginBlock?: MQ<string>;
+  marginInlineStart?: ResponsiveValue<string>;


One slight concern here is this now implies all components with logical props now support container queries and it won't be clear to users which is actually do. Maybe we just have to bite the bullet for now and the other components can be updated soon as aware this PR could keep growing.

Just a reminder we are going to need to update the docs also to reflect the change from MQ to ResponsiveValue

jps · 2023-05-16T15:02:24Z

src/utils/style/getters.ts

+  > = usedValues.reduce(
+    (acc: Record<CSSQuery<T>['rule'], string | CSSObject>, prop) => {
+      const values = {} as {[Key in keyof T]: T[Key]};
+      if (prop[1].rules) {


minor but this condition can be pulled up to a filter, to reduce nestings

jps · 2023-05-16T15:04:51Z

src/grid-layout/__tests__/grid-layout.test.tsx

@@ -55,7 +55,7 @@ describe('GridLayout', () => {
    const props: GridLayoutProps = {
      areas: `
        "A A"
-        "B C"
+      "B C"


mutebg · 2023-07-17T07:38:57Z

Since we are changing PEER DEPS I am not sure if that's not a breaking change for all users

LukeFinch added 9 commits April 24, 2023 14:49

feat(806): implement #400 container queries

a162a43

feat(806): define queries on rules object

1608c53

feat(806): extend MQ to allow CQ rules

98504da

feat(806): wip

53d1307

feat(806): update tests

1478cea

feat(806): handle responsive props container rules

2341ccc

feat(806): typescript fixes

4d78efe

feat(806): fix types update test

8f1d77e

fix(806): fix emotion package versions

d16da5a

github-actions bot added the feature This change contains a new feature label May 5, 2023

LukeFinch added 3 commits May 5, 2023 10:41

feat(806): fix type for getAreasList

754c35c

feat(806): merge main

6afb330

feat(806): change types for get typography from theme

c1ede9e

LukeFinch added 2 commits May 5, 2023 14:21

feat(806): update examples, add block story

e2f5585

Merge branch 'main' into feat/806-container-queries-storybook

99a49f3

LukeFinch self-assigned this May 5, 2023

LukeFinch added ready for review Please assist in getting this reviewed Requires design review labels May 5, 2023

LukeFinch added 2 commits May 5, 2023 15:42

feat(806): update snapshots

2a3befd

feat(806): update snapshots

d69a9f1

JohnTParsons requested changes May 5, 2023

View reviewed changes

src/block/__tests__/block.stories.tsx Outdated Show resolved Hide resolved

LukeFinch added 3 commits May 5, 2023 16:24

feat(806): address comments

71aeceb

feat(806): test coverage

1507631

Merge branch 'main' into feat/806-container-queries-storybook

3088f7c

LukeFinch removed the Requires design review label May 9, 2023

jps suggested changes May 9, 2023

View reviewed changes

mstuartf reviewed May 10, 2023

View reviewed changes

src/grid-layout/__tests__/stories/grid-layout.stories.tsx Outdated Show resolved Hide resolved

feat(806): move container props to own function

44b882f

LukeFinch added 3 commits May 10, 2023 14:02

feat(806): address comments

4a1fcd6

feat(806): remove a test

db0ddfe

Merge branch 'main' into feat/806-container-queries-storybook

a3becab

mutebg reviewed May 11, 2023

View reviewed changes

feat(806): remove unused code

f316a46

JohnTParsons previously approved these changes May 15, 2023

View reviewed changes

LukeFinch added 2 commits May 16, 2023 10:04

feat(806): move props out of overrides

80110a6

feat(806): move props out of overrides

b2425fb

LukeFinch dismissed JohnTParsons’s stale review via b2425fb May 16, 2023 09:06

jps reviewed May 16, 2023

View reviewed changes

LukeFinch added 5 commits May 17, 2023 09:28

Merge branch 'main' into feat/806-container-queries-storybook

43900b8

feat(806): test coverage, peer deps versions

ba253fc

feat(806): container props tests

5c0e7b7

feat(806): merge main

04ad044

feat(806): update package json snapshot

48cb5c5

mutebg previously approved these changes May 19, 2023

View reviewed changes

feat(806): fix merge conflicts

49d001b

LukeFinch dismissed mutebg’s stale review via 49d001b May 19, 2023 11:40

mutebg approved these changes May 19, 2023

View reviewed changes

lhaggar assigned JohnTParsons Jul 4, 2023

JohnTParsons approved these changes Jul 4, 2023

View reviewed changes

mutebg added 2 commits July 7, 2023 15:51

Merge branch 'main' into feat/806-container-queries-storybook

3f5ca77

Merge branch 'main' into feat/806-container-queries-storybook

faedeb3

mutebg added the breaking change label Jul 18, 2023

mutebg added 2 commits August 15, 2023 11:13

Merge branch 'main' into feat/806-container-queries-storybook

8a1aff9

Merge branch 'main' into feat/806-container-queries-storybook

3afbf60

mutebg requested a review from a team as a code owner October 2, 2023 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(806) container queries and storybook #877

feat(806) container queries and storybook #877

LukeFinch commented May 5, 2023 •

edited

Loading

pp-serviceaccount commented May 5, 2023

jps May 9, 2023

LukeFinch May 9, 2023

jps May 16, 2023

jps May 16, 2023

jps May 9, 2023

LukeFinch May 10, 2023

jps May 9, 2023

LukeFinch May 9, 2023

mutebg May 11, 2023

mstuartf commented May 10, 2023

mutebg May 11, 2023

LukeFinch May 12, 2023

mutebg May 15, 2023

mutebg May 11, 2023

mutebg May 11, 2023

mutebg May 11, 2023

mutebg May 11, 2023

jps May 16, 2023

jps May 16, 2023

jps May 16, 2023

mutebg commented Jul 17, 2023

feat(806) container queries and storybook #877

Are you sure you want to change the base?

feat(806) container queries and storybook #877

Conversation

LukeFinch commented May 5, 2023 • edited Loading

pp-serviceaccount commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstuartf commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mutebg commented Jul 17, 2023

LukeFinch commented May 5, 2023 •

edited

Loading