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 20, 2024 3:49pm

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-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.

src/components/Template/Template.test.tsx

src/components/Template/templateChangelogData.ts

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.

CHANGELOG.md

src/components/Template/Template.stories.tsx

src/components/Template/Template.test.tsx

src/components/Template/templateChangelogData.ts

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.

src/components/Template/Template.mdx

src/components/Template/Template.test.tsx

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!

src/components/Template/Template.mdx

src/components/Template/Template.stories.tsx

src/theme/components/template.ts

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.

I was thinking about this a bit more. Design tokens are static, while this object is responsive. From what I recall, the only other design tokens that are setup with reference to screen size are the form input height values. There's one for default (anything other than mobile) and one for mobile. If we do setup tokens for these gap values, I would like to do it using the naming conventions of the new breakpoints (small mobile, large mobile, etc.).

src/components/Template/Template.stories.tsx

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

src/components/Template/Template.mdx

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

src/components/Template/Template.mdx

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


I will work on this.

src/components/Template/Template.mdx

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

src/components/Template/Template.mdx Outdated Show resolved Hide resolved

src/components/Template/Template.stories.tsx Show resolved Hide resolved

src/theme/components/template.ts Outdated Show resolved Hide resolved

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

bigfishdesign13 added 2 commits December 19, 2024 09:42

type fix

694da56

final cleanup

8289bb3

vercel bot deployed to Preview December 20, 2024 15:34 View deployment

package-lock

ab29dce

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

bigfishdesign13 merged commit ab8e0ab into new-template-feature-branch Dec 20, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DSD-1884: new template component #1711

DSD-1884: new template component #1711

oliviawongnyc commented Dec 5, 2024 •

edited by jira bot

Loading

vercel bot commented Dec 5, 2024 •

edited

Loading

EdwinGuzman left a comment

EdwinGuzman Dec 6, 2024

EdwinGuzman Dec 6, 2024

EdwinGuzman left a comment

EdwinGuzman Dec 6, 2024

oliviawongnyc Dec 9, 2024 •

edited

Loading

bigfishdesign13 Dec 10, 2024

bigfishdesign13 Dec 10, 2024

oliviawongnyc Dec 11, 2024

bigfishdesign13 Dec 11, 2024

EdwinGuzman left a comment

EdwinGuzman Dec 9, 2024

EdwinGuzman commented Dec 9, 2024

jackiequach left a comment

bigfishdesign13 Dec 10, 2024

EdwinGuzman Dec 18, 2024

bigfishdesign13 Dec 20, 2024

bigfishdesign13 left a comment

bigfishdesign13 Dec 10, 2024

oliviawongnyc Dec 11, 2024

EdwinGuzman Dec 18, 2024

bigfishdesign13 Dec 10, 2024

oliviawongnyc Dec 11, 2024

EdwinGuzman commented Dec 18, 2024

DSD-1884: new template component #1711

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

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

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

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

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