[SkeletonPage] Rebuild with layout primitives

[chazdean]

WHY are these changes introduced?

Fixes #7714

WHAT is this pull request doing?

• Refactors SkeletonPage and its child components to use our layout primitives.
• Removes max-width: 100% property from AlphaStack
• Adds storybook examples for SkeletonPage with narrowWidth and with fullWidth props

Issues with current PR:

Unable to center `SkeletonPage` when `narrowWidth` prop is used - SOLVED ✅

Before	After

Our current `Text` component has the opposite breakpoint behaviour compared to how `.Title` is used in `SkeletonPage` - SOLVED ✅

Before	After
.Title { font-weight: var(--p-font-weight-semibold); font-size: 24px; line-height: 28px; @media #{$p-breakpoints-md-up} { font-size: 20px; } }	.headingXl { font-size: var(--p-font-size-300); line-height: var(--p-font-line-height-3); @media #{$p-breakpoints-md-up} { font-size: var(--p-font-size-400); line-height: var(--p-font-line-height-4); } }

role prop is missing from Box, creating a separate issue to solve this here SOLVED ✅

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility
• Updated the component's README.md with documentation changes
• Tophatted documentation changes in the style guide

[github-actions]

size-limit report 📦

Path	Size
polaris-react-cjs	211.16 KB (+0.06% 🔺)
polaris-react-esm	136.34 KB (+0.07% 🔺)
polaris-react-esnext	191.26 KB (-0.12% 🔽)
polaris-react-css	41.4 KB (-0.54% 🔽)

[aveline]

@chaz I created a codepen of how we should be able to use flex to center when narrowWidth is set

https://codepen.io/enileva/pen/xxzjOej

then I tried it with the component by wrapping the outer Box with this div

<div style={{display: 'flex', flexDirection: 'column', alignItems: 'center'}}>

which worked

but as we know, using AlphaStack doesn't. And then I realized it's because of the child AlphaStack with the fullWidth, the styles are getting mixed up with the parent AlphaStack that we don't want to be fullWidth. So that's a problematic implementation with fullWidth that we should revisit. Any thoughts @kyledurand?

Let's try to figure out if we can fix AlphaStack but if that drags on, we can unblock this by shipping it with the div and inline styles above?

[aveline]

I looked at AlphaStack and this is the issue

https://github.com/Shopify/polaris/blob/main/polaris-react/src/components/AlphaStack/AlphaStack.scss#L30

It's overriding the max-width on Box. So not related to fullWidth like I originally thought.

@laurkim @kyledurand any context on why we added max-width: 100%; to AlphaStack? Would it be safe to remove?

[kyledurand]

any context on why we added max-width: 100%; to AlphaStack? Would it be safe to remove?

Might have something to do with the > * {width: 100%} for fullWidth below. I think maybe it was causing overflow on the container unless max width was applied but I'm speculating. Lo would know when she's back.

For now, as an unblocker, we could remove the max width and do:

.fullWidth {
  > * {
    flex: 1 1 auto;
  }
}

And see if that causes any visual regressions. We shouldn't need to use width on a flex container these days.

OR

Try removing the fullWidth prop because I'm not sure it's used anywhere and it's kind of misleading. The flex container should take the full width anyway and messing with the children direct descendants in this way feels weird

[laurkim]

It's overriding the max-width on Box. So not related to fullWidth like I originally thought.

@laurkim @kyledurand any context on why we added max-width: 100%; to AlphaStack? Would it be safe to remove?

@aveline @kyledurand I think it was added to fix a bug on content when the fullWidth prop was passed in during the early stages of build. Should be safe to remove.

@chazdean can we add separate storybook examples for SkeletonPage with narrowWidth and with fullWidth props?

[sarahill]

Our current Text component has the opposite breakpoint behaviour compared to how .Title is used in SkeletonPage

Yea, this is one I expected we'd have to manually set as it does behave differently. Expected behavior: Page title remains 20px across breakpoints.

[sarahill]

✅

[laurkim]

I recommend decoupling the new full width and narrow width stories from this PR and just merging those into main directly, then rebasing this and layout-rebuild-batch-2 to pick up the changes (or dropping the commit to add those stories).
I tested the existing SkeletonPage without any of the layout primitive changes off latest main with those new stories added and I'm seeing a difference for the withFullWidth example. Here's a gif comparing the chromatic from this PR and me running latest main with the story locally.

Also, looks like this is exceeding size limit. Can we bump to the threshold to 220 kb?

Full width issue resolved

[laurkim]

Tophatted stories at all breakpoints against latest main running locally and lgtm! Nice 💯