DSD-1702: responsive grid system #1591
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
bigfishdesign13
changed the title
bigfishdesign13
added
the
In Progress
bigfishdesign13
changed the title
bigfishdesign13
added
Needs Review
bigfishdesign13
requested review from
EdwinGuzman,
7emansell,
dgcohen,
jackiequach,
oliviawongnyc,
isastettler,
nypl-wluisi and
charmingduchess
| Inspect responsive displays in between each breakpoint for how content responds | ||
| across the fluid spectrum | ||
|
|
||
| ## Figma Reference |
| The following terms are used through the documentation, so it is important to | ||
| understand what they mean. | ||
|
|
||
| ### Breakpoints & Breakpoint Ranges |
There was a problem hiding this comment.
Should this go in the TOC as a subpoint under "Terminology"?
There was a problem hiding this comment.
I originally had a lot of items as sub-bullets in the list, but I opted to remove them to streamline the TOC. Let's see how others feel.
There was a problem hiding this comment.
Ha! I did leave everything in there. See, I flip-flopped so much that I can't remember where I landed. But I will make some changes to try to simplify the TOC. Looking at it now, I am not happy with all those links in there.
| that in mind, the NYPL Reservoir Design System will designate four actual | ||
| breakpoints and from that we will define five breakpoint ranges: | ||
|
|
||
| #### Breakpoints |
There was a problem hiding this comment.
This and the rest in this section are in the TOC directly under the "Terminology" heading, but they are all marked with four #s. They should be three for consistent hierarchy.
There was a problem hiding this comment.
Yes, I just caught that myself.
|
|
||
| #### Breakpoint Ranges | ||
|
|
||
| - Small mobile (base): less than 480px (see Appendix Section I: Small Mobile) |
There was a problem hiding this comment.
I think you are using "base" in reference to Chakra, right? If so, then the other values in parenthesis should also be the chakra value. So "sm" instead of "s", "md" instead of "m", etc.
There was a problem hiding this comment.
Yes, I did want to mirror the Chakra syntax. I can easily fix the labels, but do you think I should even include that here?
| <Image src="/img/grid-gutters.png" alt="Grid gutters" mb="l" /> | ||
| </div> | ||
| <div> | ||
| #### Margins |
There was a problem hiding this comment.
Oddly, this is not indented like the examples above.
There was a problem hiding this comment.
VS Code was doing some weird things with the formatting when I was originally working on it, but I was now able to normalized the formatting.
src/components/Template/Template.tsx
Outdated
| @@ -188,7 +204,10 @@ const TemplateContent: React.FC<any> = ( | |||
| const TemplateContentTop: React.FC<any> = ( | |||
| props: React.PropsWithChildren<TemplateProps> | |||
| ) => { | |||
| const styles = useStyleConfig("TemplateContentTopBottom", {}); | |||
| const { useLegacyGrid } = props; | |||
There was a problem hiding this comment.
Also destructure children to update line 211.
src/styles/_01-breakpoints.scss
Outdated
| @@ -15,6 +15,12 @@ | |||
| --nypl-breakpoint-medium: 768px; | |||
| --nypl-breakpoint-large: 1024px; | |||
| --nypl-breakpoint-xl: 1280px; | |||
|
|
|||
| // Safe Area Environment Vars | |||
There was a problem hiding this comment.
Can you include that this is mostly for mobile ios?
There was a problem hiding this comment.
Actually, I will just remove those. We are are not going to them. Not yet, at least.
src/components/Template/Template.tsx
Outdated
| @@ -310,22 +332,31 @@ export const TemplateAppContainer: ChakraComponent< | |||
| renderFooterElement = true, | |||
| renderHeaderElement = true, | |||
| renderSkipNavigation = false, | |||
| useLegacyGrid = true, | |||
There was a problem hiding this comment.
Should this be set to false by default? We want consumers to use the new grid and only explicitly set the legacy if they need to?
There was a problem hiding this comment.
Yes, good catch. I missed this when I flipped the logic at the end of my dev.
src/theme/components/template.ts
Outdated
| left: ({ useLegacyGrid = false }) => ({ | ||
| gridTemplateColumns: useLegacyGrid ? { md: "255px 1fr" } : undefined, | ||
| }), | ||
| right: ({ useLegacyGrid = false }) => ({ |
There was a problem hiding this comment.
Wrap the functions in defineStyle
src/theme/components/template.ts
Outdated
| * - Small tablet: 2/3 width | ||
| * - Large tablet & desktop: 3/4 width | ||
| * */ | ||
| left: ({ useLegacyGrid = false }) => ({ |
There was a problem hiding this comment.
Wrap the functions in defineStyle
| gridColumn: "1 / -1", | ||
| }), | ||
| }); | ||
| const TemplateContent = defineStyleConfig({ | ||
| baseStyle: defineStyle({ | ||
| // Set this element to start on the second 1280px grid column. | ||
| baseStyle: defineStyle(({ useLegacyGrid = false }) => ({ |
There was a problem hiding this comment.
Is there a reason why the checks have to happen in every style definition separately, rather than once up top (eg: switch vs if-else)
There was a problem hiding this comment.
The conditions are used to set specific values for various style attributes and we've found it is easier to see the condition right on the attribute rather than abstracting the condition into a separate function elsewhere in the file.
There was a problem hiding this comment.
Is it easier because Chakra favors using one pattern over the other? Or is this an argument that an if-else ladder is an easier pattern to read in general? Or an if-else ladder is easier to read in this specific situation because ... ?
There was a problem hiding this comment.
We've continued to use patterns that Chakra shows in their docs. That being said, I am curious about the solution you are suggesting. There are a number of styles for separate elements that are dependent on the useLegacyGrid prop. What type of design pattern would you suggest to consolidate the conditions based on that prop value?
There was a problem hiding this comment.
This just comes from the principle that, if we're having a switch that we expect to remove in the future, it's easier to read and easier to remove if we define the entire legacy grid style in one place, and the entire new grid style in another place. Now, if the tools we use don't allow that (I know Storybook doesn't, but I didn't know if Chakra didn't) then we don't have much of a choice.
Fixes JIRA ticket DSD-1702
This PR does the following:
Templatecomponent to implement a new responsive grid system.Templatecomponent to explain how the new responsive grid system will work and how to use the legacy "grid" layout.responsiveGuttersandresponsiveSpacingdata objects to make available to consuming apps.Responsive Gridpage to theStyle Guide.How has this been tested?
Accessibility concerns or updates
Checklist:
Front End Review: