feat(components): Feedback on Layout 2.0 #2485

scotttjob · 2025-04-15T12:41:31Z

Motivations

Got some feedback on the Layout components.
Wanted the review of the feedback to be easy (this is a big PR), so I broke the feedback out into its own PR so it can be reviewed independently.
If we approve this PR, it will be an implied approval on the original Layout PR as well (we can rubber stamp that through, just to make things less complicated).

Changes

Updated 'space' token for 'gap' everywhere in the new layout components. There was a few references, and they've all been updated.
Removed useMediaQuery usages for 'collapseBelow'. Instead relying on common Atlantis breakpoints. Also included a 'collapsed' prop so the consumer can have completely control beyond the standard breakpoints.
Added the ability to set new layout components to specified list of semantic HTML elements (additional elements can be added in the future if needed).
Added the ability for data and aria pass through, open to alternative patterns for this approach.
Added role/id pass through for all components, along with UNSAFE styles and classnames.
Updated naming of some functions away from 'use' based react hooks to 'get' based standard functions.
Moved all Inline styles configuration out of disconnected (and prematurely optimized) useMemo instances.
Updated some descriptions to be clearer

Changes can be
tested via Pre-release

In Atlantis we use Github's built in pull request reviews.

…, semantic elements

cloudflare-workers-and-pages · 2025-04-15T12:48:39Z

Deploying atlantis with Cloudflare Pages

Latest commit:	`74ab561`
Status:	✅ Deploy successful!
Preview URL:	https://b1317577.atlantis.pages.dev
Branch Preview URL:	https://scott-t-layout-feedback.atlantis.pages.dev

View logs

ericchernuka

Overall liking the direction and flexibility. Few questions.

ericchernuka · 2025-04-15T13:42:39Z

packages/components/src/Cluster/types.ts

+   * More information in the [Customizing components Guide](https://atlantis.getjobber.com/guides/customizing-components).
+   */
+  readonly UNSAFE_className?: {
+    container?: string;


I had a question on this? I realize its consistent, but requires an object to be created for elements with only 1 component like this? Same comment about styles.

Yeah I went for consistency over what felt like the shortest path. I'll double check with the team to see if the string-only option is acceptable within the pattern, because I would prefer it myself for this.

I think it could become a clear rule that if it has more than one override, then it's an object; otherwise, it's a string.

@jdeichert, @ZakaryH & I have discussed this before when adding UNSAFE_ to atom components that are also just a container. Jacob's suggestion was that we used a keyed map to make sure we're handling future scenarios/future proofing (even if it seems unlikely any other sub element will be added to a component).
I did initially have the same thoughts, and we could have just a string for className and an object for _styles if people feel strongly. I tend to lean into consistency at this point.

That's fair. I think it probably speaks to components needing more composition so that the overrides could be targetted, but another way to approach that with the current structure would be to actually give data-slot="<componentName>" which could then be targetted by the CSS through data attribute selectors rather than needing to be the ones responsible for plumbing classnames through.

Forgive the tailwind nuances here that they're hiding behind Components, but he presents an interesting approach to controlling styles from above using data attributes. https://www.youtube.com/watch?v=MrzrSFbxW7M

I was concerned about future-proofing because there are cases where we have to go in and wrap an extra div sometimes.

However, I'm realizing that typescript could allow us to support both scenarios. We could start off with a string type, and IF we ever need to expand this in the future, we could also accept an object with keys.

I can't remember if I had any other pro-object arguments, but I'm less concerned now.

Wonder if we just leverage what Braid does? https://github.com/seek-oss/braid-design-system/blob/0de668c186fd972a197160b799b980243ef58dc7/packages/braid-design-system/src/lib/components/private/buildDataAttributes.ts

Then you don't need to re-type data?

ericchernuka · 2025-04-15T13:43:21Z

packages/components/src/Cover/Cover.tsx

 }) {
-  return <div className={styles.centerContent}>{children}</div>;
+  return (
+    <div


Should this also have a Tag?

Sure should! Nice catch.

ericchernuka · 2025-04-15T13:44:59Z

packages/components/src/sharedHelpers/getCommonProps.ts

+export function dataPropsMapped(data?: Record<string, string>) {
+  return Object.entries(data || {}).map(([key, value]) => ({
+    [key]: value,
+  }));
+}


Is data supposed to be used like

data={{ 'data-foo': true }}

or

data={{ foo: true }}

Guess the other option is to just extend DataHTMLAttributes from React?

I updated this while you were reviewing (my spidey sense was tingling)! it's now a proper data- attribute and it lets you know how to use it.

@scotttjob instead of Record<string, string> you can possibly make a typescript key type that checks the prefix and this will guarantee it only allows data- keys

I can hack up something today as an example 👍

interface DataAttributes { [key: `data-${string}`]: string; } export function dataPropsMapped(data?: DataAttributes) { return Object.entries(data || {}).map(([key, value]) => ({ [key]: value, })); }

If you use this DataAttributes type as the data prop on components, it will flow through here properly, and the prop will block incorrect usages of it!

I noticed you already used data?: { [key: data-${string}]: string }; in CommonAtlantisProps. Ignore me 😅

@ericchernuka something I learned about DataHTMLAttributes is that it pulls in a lot more attributes than you would expect since it also extends HTMLAttributes (which extends AriaAttributes and DOMAttributes).

I believe that's because it's for the data element: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/data (just like how InputHTMLAttributes is for input)

It's not actually data attributes as far as I can tell

Yup. I dug into the types and was like why am I getting aria attributes!?

ericchernuka · 2025-04-15T13:45:21Z

packages/components/src/sharedHelpers/getCommonProps.ts

+export function ariaPropsMapped(aria?: React.AriaAttributes) {
+  return Object.entries(aria || {}).map(([key, value]) => ({
+    [key]: value,
+  }));
+}


Same comment about aria attributes.

Wonder if we can just extend AriaAttributes from React?

Donezo! This was one was in flight too while you were reviewing. You can now pass fully qualified aria- and it autocompletes for you.

ericchernuka · 2025-04-15T13:47:41Z

packages/components/src/sharedHelpers/getMappedBreakpointWidth.ts

+export const AtlantisBreakpoints = {
+  xs: 0,
+  sm: 490,
+  md: 768,
+  lg: 1080,
+  xl: 1440,
+};


I noticed that we seem to redefine these across many components? Do we need a central version that they all extend from?

Yeah it's going to be this one. We seemed to have a pattern in the past where we didn't re-use anything across components unless they got promoted to hooks, but that's too much effort for some of these component-only usages.

taylorvnoj · 2025-04-15T14:34:34Z

Looks really good to me. We have been adding UNSAFE_ documentation on Implement tabs but that and other Implement tab type info can definitely be in a follow up. I might even be able to do some of that work during my support hero rotation that starts today.
Not sure if you have any other changes for this PR - ping me for when ready/when circleci is green 👍🏻

ericchernuka · 2025-04-15T14:12:18Z

packages/components/src/sharedHelpers/types.ts

+  /** Standard HTML id attribute. */
+  id?: string;
+}
+export type CommonAllowedElements =


Let's also add p which would be useful in some scenerios

ericchernuka · 2025-04-15T14:16:55Z

packages/components/src/Cluster/types.ts

+   * More information in the [Customizing components Guide](https://atlantis.getjobber.com/guides/customizing-components).
+   */
+  readonly UNSAFE_className?: {
+    container?: string;


That's fair. I think it probably speaks to components needing more composition so that the overrides could be targetted, but another way to approach that with the current structure would be to actually give data-slot="<componentName>" which could then be targetted by the CSS through data attribute selectors rather than needing to be the ones responsible for plumbing classnames through.

Forgive the tailwind nuances here that they're hiding behind Components, but he presents an interesting approach to controlling styles from above using data attributes. https://www.youtube.com/watch?v=MrzrSFbxW7M

ericchernuka · 2025-04-15T14:43:32Z

packages/components/src/Cluster/types.ts

+   * More information in the [Customizing components Guide](https://atlantis.getjobber.com/guides/customizing-components).
+   */
+  readonly UNSAFE_className?: {
+    container?: string;


Wonder if we just leverage what Braid does? https://github.com/seek-oss/braid-design-system/blob/0de668c186fd972a197160b799b980243ef58dc7/packages/braid-design-system/src/lib/components/private/buildDataAttributes.ts

Then you don't need to re-type data?

nad182 · 2025-04-15T15:36:26Z

packages/components/src/SideKick/types.ts

  readonly children: React.ReactNode;
  /** The width of the sidekick. */
  readonly sideWidth?: string;
  /** The minimum width of the content. */
  readonly contentMinWidth?: string;
  /** The amount of space between the sidekick and the content. Semantic tokens are available. */
-  readonly space?: string | Spaces;
+  readonly gap?: string | Spaces;


I see that in other places you used a slightly different type:

readonly gap?: Spaces | (string & NonNullable<unknown>);

Is it intentionally different here?

Not intentionally no! I've replaced these various Spaces | options with a new 'GapSpacing' type that should be used everywhere this loose type appeared before.

nad182 · 2025-04-15T15:37:48Z

packages/components/src/Stack/types.ts

  readonly children: React.ReactNode;
  /** The amount of space between the children. Semantic tokens are available. */
-  readonly space?: string | Spaces;
-  /** The number of children to split the stack after (1-15). Requires parent to have height greater than the sum of the children. */
+  readonly gap?: string | Spaces;


Same comment as above regarding the type.

Replaced with the above!

nad182 · 2025-04-15T15:39:14Z

packages/components/src/Tiles/types.ts

  readonly children: React.ReactNode;
  /** The minimum size of the tiles. */
  readonly minSize: string;
  /** The amount of space between the tiles. Semantic tokens are available. */
-  readonly space: string | Spaces;
+  readonly gap: string | Spaces;


Same as above. Also is gap required for Tiles?

Gap is not required! Good catch. I've updated this one as well with the new type

jdeichert

Looks good! I'll approve the main PR once this one merges down.

…is into scott_t/layout-feedback

scotttjob added 8 commits April 14, 2025 11:56

updating space for ga

4f3d2d5

semantic tags in the layout components

2ac4b88

allowing passthrough of data, aria, id, and role.

cc584c1

unsafe styles and classnames. no more media queries, prop passthrough…

8099a86

…, semantic elements

final cleanup on layout addressing

94bf678

adding some additional feedback

1de7017

actually making these new props work

4a7abc5

readding collapse below

3cf09dd

scotttjob requested a review from a team as a code owner April 15, 2025 12:41

jobbiebot bot added review requested Team UX Foundations labels Apr 15, 2025

scotttjob added 3 commits April 15, 2025 09:52

adding missing styles

28c7dc1

updating common types to be more specific

52fc7e4

making types a bit more specific and adding js doc comments for them

837885b

ericchernuka reviewed Apr 15, 2025

View reviewed changes

updating Cover.Center to follow the same patterns as the rest

709beb0

adding better types

8e127c0

ericchernuka reviewed Apr 15, 2025

View reviewed changes

scotttjob added 3 commits April 15, 2025 12:04

removing unnecessary processing step

2f4fea0

removing unnecessary processing step

58c2d05

updating prop name

c1049c4

nad182 reviewed Apr 15, 2025

View reviewed changes

Add andText to story default args

a660b4a

jdeichert approved these changes Apr 15, 2025

View reviewed changes

jobbiebot bot added approved and removed review requested labels Apr 15, 2025

scotttjob added 2 commits April 15, 2025 14:15

adding the latest item

a8b740d

Merge branch 'scott_t/layout-feedback' of github.com:GetJobber/atlant…

74ab561

…is into scott_t/layout-feedback

taylorvnoj self-requested a review April 15, 2025 18:11

jobbiebot bot added the review requested label Apr 15, 2025

taylorvnoj approved these changes Apr 15, 2025

View reviewed changes

jobbiebot bot removed the review requested label Apr 15, 2025

scotttjob merged commit 0daa163 into scott_t/layouts Apr 15, 2025
10 checks passed

scotttjob deleted the scott_t/layout-feedback branch April 15, 2025 18:17

feat(components): Feedback on Layout 2.0 #2485

feat(components): Feedback on Layout 2.0 #2485

Uh oh!

Conversation

scotttjob commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivations

Changes

Uh oh!

cloudflare-workers-and-pages bot commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying atlantis with Cloudflare Pages

Uh oh!

ericchernuka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taylorvnoj Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeichert Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeichert Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeichert Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taylorvnoj commented Apr 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scotttjob commented Apr 15, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Apr 15, 2025 •

edited

Loading

taylorvnoj Apr 15, 2025 •

edited

Loading

jdeichert Apr 15, 2025 •

edited

Loading

jdeichert Apr 15, 2025 •

edited

Loading

jdeichert Apr 15, 2025 •

edited

Loading