DSD-1884: new template component #1711

oliviawongnyc · 2024-12-05T20:41:13Z

Fixes JIRA ticket DSD-1884 and DSD-1885

This PR does the following:

Updates Template wrapper component and children components
Adds concept of narrow main content for long text
Updates styles to make responsive gaps and sidebar in grid

How has this been tested?

In Storybook

Accessibility concerns or updates

Documentation not complete

Accessibility Checklist

Checked Storybook's "Accessibility" tab for color contrast and other issues.
The feature works with keyboard inputs including tabbing back and forward and pressing space, enter, arrow, and esc keys.
For hidden text or when aria-live is used, a screenreader was used to verify the text is read.
For features that involve UI updates and focusing on DOM refs, focus management was reviewed.
The feature works when the page is zoomed in to 200% and 400%.

Open Questions

Checklist:

I have updated the Storybook documentation and changelog accordingly.
I have added relevant accessibility documentation for this pull request.
All new and existing tests passed.

Front End Review:

Review the Vercel preview deployment once it is ready.

vercel · 2024-12-05T20:41:16Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nypl-design-system	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 11, 2024 5:55pm

EdwinGuzman

You can remove "new" from the component and file names since this is a breaking change update.

CHANGELOG.md

src/components/Template/Template.mdx

EdwinGuzman · 2024-12-06T15:46:31Z

src/components/Template/Template.mdx

-  language="jsx"
-/>
+The `TemplateMain` and `TemplateMainNarrow` components render the content as a
+`<main>` element with a default "id" of `"mainContent"`. This should be used as


Because of the same shared id value, I think it's fine to explicitly say to use either one but not both.

EdwinGuzman · 2024-12-06T16:01:36Z

src/components/Template/Template.test.tsx

-        <TemplateFooter>{footer}</TemplateFooter>
-      </Template>
-    );
+    const { container } = render(<>{templateChildren}</>);


I think it'd be good to make templateChildren a function and pass in the value of the sidebar, either left or right. Then you can have two accessibility tests and verify both sidebar variants (and the default without a sidebar) are all accessible.

EdwinGuzman · 2024-12-06T16:03:00Z

src/components/Template/Template.test.tsx

-            <TemplateContentPrimary>{contentPrimary}</TemplateContentPrimary>
-          </TemplateContent>
-          <TemplateFooter>{footer}</TemplateFooter>
+        <Template sidebar={sidebar}>


Similar comment here, this is a snapshot for the left sidebar but have snapshots for other variants.

EdwinGuzman · 2024-12-06T16:03:28Z

src/components/Template/Template.test.tsx

-        />
-      )
-      .toJSON();
-    const withChakraProps = renderer


This is still applicable.

EdwinGuzman · 2024-12-06T16:03:43Z

src/components/Template/Template.test.tsx

-        />
-      )
-      .toJSON();
-    const withOtherProps = renderer


This and the ref test below are still applicable.

EdwinGuzman · 2024-12-06T16:08:27Z

src/components/Template/templateChangelogData.ts

+    type: "Update",
+    affects: ["Functionality", "Styles"],
+    notes: [
+      "Major update to component and styles based on the [Template TAD](https://docs.google.com/document/d/1vZJX7Y-DnEM44-iWw5qoXGdxqavHEN6prhr-YDTNr0o/edit?pli=1&tab=t.0).",


This doesn't render as a link in storybook since it can't parse mdx strings. You can leave out the link.

EdwinGuzman

The update looks good! Some minor things including fixing the story:

I will do a closer test next week by trying to add different components and layouts in my local set up.

EdwinGuzman · 2024-12-06T22:13:01Z

CHANGELOG.md

@@ -8,6 +8,10 @@ Currently, this repo is in Prerelease. When it is released, this project will ad

 ## Prerelease

+### Updates
+
+- [Updates `Template` component(s)](<(https://newyorkpubliclibrary.atlassian.net/browse/DSD-1884)>) and styles per TAD.


Did you mean to have the internal <( and )>?

Don't think so!

EdwinGuzman · 2024-12-06T22:19:42Z

src/components/Template/Template.stories.tsx

+            type="informative"
+          />
+        </TemplateTop>
+        <TemplateSidebar>


You need to add a case for when "sidebar" is set to "none"

Actually, this also displays correctly when sidebar is either "left" or "right", but we do want to recommend that the DOM follows the styles as well. So if sidebar="right", the DOM should be set in the same manner.

EdwinGuzman · 2024-12-06T22:23:14Z

src/components/Template/Template.test.tsx

-    // The `<header>` HTML element has the same meaning as `role="banner"`.
-    expect(screen.getAllByRole("banner")).toHaveLength(1);
-  });
+  it("renders each section with left sidebar", () => {


nit: this should say right

EdwinGuzman · 2024-12-06T22:23:30Z

src/components/Template/Template.test.tsx

-});
+const contentBottom = <Placeholder variant="short">Content Bottom</Placeholder>;
+
+const templateComponents = (


EdwinGuzman · 2024-12-06T22:23:49Z

src/components/Template/Template.test.tsx

+    const { container } = render(templateComponents("left"));
    expect(await axe(container)).toHaveNoViolations();


Add two similar tests for the default and "right".

EdwinGuzman · 2024-12-06T22:25:25Z

src/components/Template/templateChangelogData.ts

+    version: "Prerelease",
+    type: "Update",
+    affects: ["Functionality", "Styles"],
+    notes: ["Major update to component and styles based on the Template TAD."],


It's ok to be more descriptive here! Maybe not all the details but definitely the major ones.

EdwinGuzman · 2024-12-06T22:26:06Z

src/theme/components/template.ts

+    m: "0 auto",
+    p: "s",
+    gridTemplateAreas: `"breakout" "top" "main" "bottom"`,
+    gridTemplateColumns: "repeat(1, minmax(100px, 1fr))",


How did you get 100px for the first value in minmax?

Arbitrarily, though I think it looks good.... it should probably be decided on by @bigfishdesign13.

The smallest screen width that designers consider (in theory) is 320px, so I'd suggest changing the min here to 320px.

Why use repeat when it's not repeating?

Actually, I'd argue that minmax should be removed altogether from the column styles and instead apply minWidth: "320px" to the full grid.

If the smallest screen width is 320, then the minWidth of the grid should be smaller than that, no?

@oliviawongnyc Yes. That is what I was referring to during our call earlier. It should be 320 minus the value of the padding. For the mobile viewport the padding on either side is 16px, so the minWidth for the grid should be set to "288px".

320px - (16px x 2) = 288px

EdwinGuzman

Minor comments. I will approve once others get a chance to review the PR (I think I've been looking so often that at this point I will miss things).

Important: make a feature branch and point this PR into that branch. This should not accidentally be merged into development and this feature won't go out until 4.0 is released.

EdwinGuzman · 2024-12-09T20:23:33Z

src/components/Template/Template.mdx

@@ -2,499 +2,121 @@ import { ArgTypes, Canvas, Description, Meta, Source } from "@storybook/blocks";
 import ComponentChangelogTable from "../../utils/ComponentChangelogTable";
 import { changelogData } from "./templateChangelogData";

-import * as TemplateStories from "./Template.stories";
+import * as TemplateNewStories from "./Template.stories";


nit: remove "new" where necessary.

EdwinGuzman · 2024-12-09T20:25:11Z

src/components/Template/Template.test.tsx

+    expect(await axe(container)).toHaveNoViolations();
+  });
+
+  it("passes axe accessibility test", async () => {


These four tests have the same description. Update this so they are all descriptive to the test.

EdwinGuzman · 2024-12-09T20:26:16Z

src/components/Template/Template.test.tsx

+  <Template sidebar={sidebar}>
+    <TemplateBreakout>{breakout}</TemplateBreakout>
+    <TemplateTop>{contentTop}</TemplateTop>
+    {sidebar !== "none" && !useMainNarrow && (


Not too big of a deal since this is a test, but if "sidebar" is "right", the DOM will not be in the right order.

EdwinGuzman · 2024-12-09T20:29:50Z

@jackiequach @7emansell @bigfishdesign13 if you get a chance to review, please try to before 12/11.

jackiequach

The "Template Full Example" is very cramped at 600px screen width. Otherwise, this looks great!

jackiequach · 2024-12-10T15:37:10Z

src/components/Template/Template.mdx

-should be wrapped in a `<div>` element or a `<Box>` component.
-
-Basic "template" components structure:
+### Using the `Template` with the `Header` and `Footer`


Add a short description here for better clarity

jackiequach · 2024-12-10T16:01:29Z

src/components/Template/Template.stories.tsx

+              <Button id="submit">Submit</Button>
+            </FormField>
+          </Form>
+          <Table


Unrelated issue: this table does not scroll properly at 600px screen width

Hmm, for me, the table scrolls at 600px, but you're right that it doesn't look great.

@bigfishdesign13 & @EdwinGuzman, will this be resolved with the new md breakpoint? I believe it's 768px, meaning this would be one column up until then.

Container queries would resolve this issue. The Table will not use a fixed first column for mobile viewports, but what you are seeing can happen when it is dropped in a narrow space. The new mobile breakpoint of 768px will help, but it will still not be ideal.

Another way to improve the UX here would be to turn off the row headers on the Table when the viewport width drops below 960px.

Yes, good points. Won't do anything about it here though.

jackiequach · 2024-12-10T16:03:04Z

src/theme/components/template.ts

- * Grid layout based on https://www.joshwcomeau.com/css/full-bleed/
- */
+
+export const responsiveGap = { base: "1rem", md: "1.5rem", xl: "1rem" };


is xl supposed to be 2rem?

I don't believe so, but @bigfishdesign13, could you please confirm?

It looks like the values for responsiveGap and responsivePadding were flip-flopped. For responsiveGap, the xl value should be "2rem". And for responsivePadding, the xl value should be "1rem".

bigfishdesign13 · 2024-12-10T18:32:31Z

src/theme/components/template.ts

- */
+
+export const responsiveGap = { base: "1rem", md: "1.5rem", xl: "1rem" };
+export const responsivePadding = { base: "1rem", md: "1.5rem", xl: "2rem" };


To keep proper alignment in the UI, this same type of responsive padding will be used in a number of other components. The responsive gaps could also be used in some places. Do you think it would make sense to move these into global.ts?

Yes, could this also be a general "DS gap" and "DS padding" set of values that can be used elsewhere? Perhaps exported as design tokens if consuming apps need them.

bigfishdesign13 · 2024-12-10T18:57:36Z

src/components/Template/Template.stories.tsx

+              <Button id="submit">Submit</Button>
+            </FormField>
+          </Form>
+          <Table


Container queries would resolve this issue. The Table will not use a fixed first column for mobile viewports, but what you are seeing can happen when it is dropped in a narrow space. The new mobile breakpoint of 768px will help, but it will still not be ideal.

Another way to improve the UX here would be to turn off the row headers on the Table when the viewport width drops below 960px.

bigfishdesign13

Some suggestions.

bigfishdesign13 · 2024-12-10T20:29:08Z

src/theme/components/template.ts

  baseStyle: defineStyle({
-    gridColumn: { base: "1", md: "1 / span 2" },
-    paddingX: "s",
+    maxWidth: { lg: "720px" },


Let's see if we can bring this into alignment with the 12-column grid. When using a 12-column grid at 1280px with appropriate padding and gaps, an eight column span is 822px. And interestingly, DC is using 820px max width on the About page.

The underlying grid for desktop would use:

gridTemplateColumns: "1fr 4fr 1fr"

And the content area would be the 4fr column.

For tablet, maybe something like this:

gridTemplateColumns: "1fr 10fr 1fr"

And the next jump would just be "1fr" (full width).

Thoughts?

I won't be able to work on this myself, but I think we can achieve this in TemplateMainNarrow by adding display: grid and something like gridTemplateColumns: "2fr minmax(800, 8fr) 2fr". This second value would need to be fiddled with.

Then, the component needs to have another div wrapper like so:

const TemplateMainNarrow: React.FC<React.PropsWithChildren<TemplateMainProps>> = ({ children, id = "mainContent" }) => { const styles = useStyleConfig("TemplateMainNarrow"); return ( <Box as="main" id={id} gridArea="main" __css={styles}> <div style={{ gridColumn: 2 }}>{children}</div> </Box> ); };

Ticket: https://newyorkpubliclibrary.atlassian.net/browse/DSD-1903

bigfishdesign13 · 2024-12-10T21:25:36Z

src/components/Template/Template.mdx

+Wide lines of text are difficult to read and are proven to make it harder for
+people to focus, while short lines of text force the eye to travel back too often,
+breaking a reader's rhythm. The `TemplateMain` component has a max width of 1280px,
+which is too wide for text. If your content includes long chunks of text, you
+should use the `TemplateMainNarrow` component instead.


We need to mention that this type of scenario is only applicable when the sidebar is not used.

bigfishdesign13 · 2024-12-10T21:30:51Z

src/components/Template/Template.mdx

-`}
-  language="jsx"
-/>
+TBD.


I will work on this.

src/components/Template/Template.mdx

bigfishdesign13 · 2024-12-10T21:38:43Z

src/components/Template/Template.mdx

-`}
-  language="jsx"
-/>
+## Template Sidebar Configuration


We should either expand this section to detail the placement and function of all of the grid regions or we should add a section for each region. I am leaning toward adding separate sections to describe each region.

Okay, I think that makes sense! Will leave this work for the documentation ticket.

EdwinGuzman · 2024-12-18T20:22:53Z

I'm going to merge this PR in and we can work on subsequent fixes in future PRs as planned.

oliviawongnyc added 5 commits December 3, 2024 10:05

Creates new Template files

3ff4df8

Style updates

f911d19

Cleans up styles

e29688c

Cleans up docs and tests

2ad0f66

Merges in development, updates CHANGELOG

15a9ee9

vercel bot deployed to Preview December 5, 2024 20:42 View deployment

Updates snapshot

d38e8b2

vercel bot deployed to Preview December 5, 2024 20:49 View deployment

oliviawongnyc requested review from EdwinGuzman and bigfishdesign13 December 5, 2024 20:51

oliviawongnyc added DON’T MERGE YET When somethign has been approved, but should not be merged just yet. Needs Review Pull requests that are ready for peer review. labels Dec 5, 2024

Adds comments and types

6db6813

vercel bot deployed to Preview December 5, 2024 21:23 View deployment

Cleans up comments

bee3094

vercel bot deployed to Preview December 5, 2024 21:28 View deployment

Cleans up stories

7a4da94

vercel bot deployed to Preview December 5, 2024 21:35 View deployment

EdwinGuzman requested review from jackiequach and 7emansell December 6, 2024 15:35

EdwinGuzman requested changes Dec 6, 2024

View reviewed changes

Updates per PR feedback

dc3c8cf

oliviawongnyc requested a review from EdwinGuzman December 6, 2024 21:55

vercel bot deployed to Preview December 6, 2024 21:56 View deployment

EdwinGuzman reviewed Dec 6, 2024

View reviewed changes

More updates per feedback

0129f2c

vercel bot deployed to Preview December 9, 2024 17:32 View deployment

oliviawongnyc requested a review from EdwinGuzman December 9, 2024 18:03

EdwinGuzman reviewed Dec 9, 2024

View reviewed changes

jackiequach reviewed Dec 10, 2024

View reviewed changes

Updates per PR feedback

22f5641

vercel bot deployed to Preview December 10, 2024 17:21 View deployment

oliviawongnyc changed the base branch from development to new-template-feature-branch December 10, 2024 17:30

Fixes test, tweaks per PR feedback

8308057

vercel bot deployed to Preview December 10, 2024 17:52 View deployment

oliviawongnyc requested a review from EdwinGuzman December 10, 2024 17:53

Updates docs

014f3c1

vercel bot deployed to Preview December 10, 2024 17:58 View deployment

bigfishdesign13 reviewed Dec 10, 2024

View reviewed changes

oliviawongnyc added 2 commits December 11, 2024 12:30

Updates per PR feedback

5f322c1

Styling tweaks

d579b02

vercel bot deployed to Preview December 11, 2024 17:55 View deployment

bigfishdesign13 added the v4 Updates for the v4.0 feature branch label Dec 16, 2024

bigfishdesign13 changed the title ~~DSD-1884/new template component~~ DSD-1884: new template component Dec 16, 2024

		const { container } = render(templateComponents("left"));
		expect(await axe(container)).toHaveNoViolations();

DSD-1884: new template component #1711

Are you sure you want to change the base?

DSD-1884: new template component #1711

Conversation

oliviawongnyc commented Dec 5, 2024 • edited by jira bot Loading

This PR does the following:

How has this been tested?

Accessibility concerns or updates

Accessibility Checklist

Open Questions

Checklist:

Front End Review:

vercel bot commented Dec 5, 2024 • edited Loading

EdwinGuzman left a 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

EdwinGuzman left a 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

Choose a reason for hiding this comment

oliviawongnyc Dec 9, 2024 • edited Loading

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

EdwinGuzman left a 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

EdwinGuzman commented Dec 9, 2024

jackiequach left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigfishdesign13 left a 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

EdwinGuzman commented Dec 18, 2024

oliviawongnyc commented Dec 5, 2024 •

edited by jira bot

Loading

vercel bot commented Dec 5, 2024 •

edited

Loading

oliviawongnyc Dec 9, 2024 •

edited

Loading