[new DL][Skeleton Page] Ensure Skeleton Page title is the same font size as title rendered in Page component #3449

francinen · 2020-10-09T16:32:06Z

WHY are these changes introduced?

When the new design language is enabled, the page title shrinks when the Skeleton Page component is replaced by the Page component.

The font size for the page title should be consistent between the Skeleton Page component and the Page component.

WHAT is this pull request doing?

This PR implements a solution similar to the one introduced by #3468. Instead of using JavaScript to conditionally render small or large Display Text when the navigation is collapsed, the Skeleton Page component now relies purely on CSS to set different font sizes based on specific breakpoints.

New design language	Current design language (no regressions)

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Testing in Storybook

Run dev s in the Polaris React repo to open Storybook
Look at the Skeleton Page examples and ensure there are no regressions

Page title defined	Page title undefined

Testing against a Shopify Web

Check out this branch and configure Shopify Web to consume your local Polaris React repo
Visit a page that uses the Skeleton Page component (e.g., /admin/apps, /admin/products, /admin/dashboards)

Viewport width	Page title defined	Page title undefined
Mobile
Desktop

🎩 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
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

github-actions · 2020-10-09T16:34:28Z

🟢 This pull request modifies 4 files and might impact 1 other files.

Details:

All files potentially affected (total: 1)

/src/components/SkeletonPage/SkeletonPage.tsx

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/SkeletonPage/SkeletonPage.scss (total: 1)

Files potentially affected (total: 1)

/src/components/SkeletonPage/SkeletonPage.tsx

🧩 src/components/SkeletonPage/SkeletonPage.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/SkeletonPage/tests/SkeletonPage.test.tsx (total: 0)

Files potentially affected (total: 0)

francinen · 2020-10-09T17:51:35Z

Does anyone know why small Display Text is being rendered when the navigation is collapsed, even though we're passing in "large" as the size prop?

cc @kyledurand

kyledurand · 2020-10-09T19:52:26Z

Does anyone know why small Display Text is being rendered when the navigation is collapsed

I wonder if this is because the media query provider doesn't yet know the size of the viewport. @BPScott would probably know the answer for sure.

Ben could this be caused by SSR? @francinen does this ever get display text large when the nav is collapsed? Or does it flash small and then switch to large?

francinen · 2020-10-09T20:06:03Z

@kyledurand When the navigation is collapsed, the display text switches from small to large as the Skeleton Page is replaced by the Page component.

BPScott · 2020-10-09T21:06:03Z

I wonder if this is because the media query provider doesn't yet know the size of the viewport. @BPScott would probably know the answer for sure.
Ben could this be caused by SSR? @francinen does this ever get display text large when the nav is collapsed? Or does it flash small and then switch to large?

Yeah this sounds right to me. When the HTML is rendered on the server-side it doesn't doesn't have any concept of media-queries or window.matchMedia (or even window!). You're using {isNavigationCollapsed} = useMediaQuery(), taking a look at MediaQueryProvider and following that into breakpoints.tsx we see that sets isNavigationCollapsed based on a media query, and on the server where no window.matchMedia is available it looks like it defaults to returning "false" - same as on a large screen (>768px) in a browser.

If our aim here is to match what the Page Header Title component does then it looks like we're good as that uses useMediaQuery to toggle the size of the DisplayText too. But there might be a better way (depending on how much effort you want to put in here)...

To get up on a pedestal for a moment: IMO MediaQueryProvider is an anti-pattern because it causes issues like this where there ends up being html mismatches between the client and the server. Changing html content based upon browser width has the potential to make the browser do lots of work and we should trry to avoid it where possible, instead we should rely on good old CSS, and where feasible always render the same HTML but show/hide it with CSS media queries.

Ideally instead of doing <DisplayText size={newDesignLanguage ? (displaySize = isNavigationCollapsed ? 'large' : 'small') : 'large'}> in both Page Header Title and in SkeletonPage we should create a new css class and style it with our scss mixins, so that we don't need to use useMedia() at all. Something like the following (typed but not tested) in Title:

.TitleText {
  @include text-style-display-large;

  .newDesignLanguage  {
    @include frame-when-nav-displayed {
      @include text-style-display-small;
  }
}

and then something similar in SkeletonPage to mimic those box heights.

francinen · 2020-10-13T13:45:19Z

@BPScott The CSS approach sounds a lot more scalable. I can give that a try later today. Thank you!

BPScott · 2020-10-13T17:40:25Z

Related: #3468.

Looks like that's ended up with a similar approach to what I talked about above (though it seems the breakpoint we want to use has changed)

francinen · 2020-10-14T18:12:15Z

@kyledurand @BPScott This PR is officially ready for review

BPScott · 2020-10-14T18:24:35Z

src/components/SkeletonPage/SkeletonPage.tsx

-  return <div className={styles.Title}>{titleContent}</div>;
+  }
+
+  return <SkeletonDisplayText size="large" />;


Does this <SkeletonDisplayText size="large"> have the same visual height as the new design language title at all viewport sizes?

I've not confirmed this but I'm guessing it doesn't as <DisplayText size="large"> matches <SkeletonDisplayText size="large"> and @alex-page wrote custom css instead of using <DisplayText size="large"> in Page. (Alternatively <SkeletonDisplayText size="large"> and thus <DisplayText size="large"> matches the new header css in Page in which case we should just use DisplayText instead of writing custom css in Page)

We should make sure this Skeleton content is the same height so we don't trigger layout shifts here too.

I have the same concern too, it looks like this change would only match what we have in Title if the title was passed in to skeleton page. Otherwise you'd always be getting display text which I don't think matches perfectly.

Some of the gifs added to the PR description show the transition from Skeleton Display Text to Display Text:

Looks like <SkeletonDisplayText size="large"> uses the display-large line heights for its heights - 28px base, 32px when-typography-not-condensed, while Page title / newDesignLanguageTitle uses 28px at all sizes) so these are indeed different.

That looks about right with the jump seen in those gifs.

Would you be interested in fixing this up so the Skeleton height matches real content so we don't get page jumps there? (uugh it looks like we currently get a jump in that first gif because the actions are taller than the title but that feels slightly out of scope for this ticket).

A div with a class like this should do the job (written but not tested):

// duplicated from SkeletonDisplayText.scss $skeleton-display-text-max-width: rem(120px); .newDesignLanguageSkeletonTitle { @include skeleton-shimmer; @include skeleton-content; max-width: $skeleton-display-text-max-width; height: rem(28px); }

francinen · 2020-10-19T14:28:21Z

@BPScott or @kyledurand Are either of you able to re-review this PR? I responded to the question re: the Skeleton Display Text height here: #3449 (comment)

kyledurand

Code looks good 👍

Looks like there might still be some content jumpiness in the second gif but this is definitely a solid improvement 👏

Thanks @francinen

BPScott

Add a comment inline about Skeleton heights with a possible solution.

@kyledurand / @alex-page: Any strong thoughts on if this should be done in this PR or if this can be done in a separate PR?

BPScott · 2020-10-20T17:33:45Z

src/components/SkeletonPage/SkeletonPage.tsx

-  return <div className={styles.Title}>{titleContent}</div>;
+  }
+
+  return <SkeletonDisplayText size="large" />;


Looks like <SkeletonDisplayText size="large"> uses the display-large line heights for its heights - 28px base, 32px when-typography-not-condensed, while Page title / newDesignLanguageTitle uses 28px at all sizes) so these are indeed different.

That looks about right with the jump seen in those gifs.

Would you be interested in fixing this up so the Skeleton height matches real content so we don't get page jumps there? (uugh it looks like we currently get a jump in that first gif because the actions are taller than the title but that feels slightly out of scope for this ticket).

A div with a class like this should do the job (written but not tested):

// duplicated from SkeletonDisplayText.scss $skeleton-display-text-max-width: rem(120px); .newDesignLanguageSkeletonTitle { @include skeleton-shimmer; @include skeleton-content; max-width: $skeleton-display-text-max-width; height: rem(28px); }

francinen · 2020-10-26T14:49:05Z

Thanks, @BPScott! How does this look?

…en new design language is enabled

alex-page

This looks good to me. @BPScott @kyledurand anything stopping us from merging?

kyledurand · 2020-10-28T16:14:46Z

Nope 🚢 !

BPScott · 2020-10-28T18:35:56Z

There's still a bit of a vertical jump (note the top bar of skeleton tab bar in the gif) but i think that's from the action buttons being taller than the title text now, which feels like a separate issue. Happy to 🚢 as is! Thanks for putting up with us @francinen!

…en new design language is enabled (#3449) Co-authored-by: Alex Page <alex@alexpage.com.au>

francinen requested a review from kyledurand October 9, 2020 17:51

francinen changed the title ~~[new DL][Skeleton Page] Ensure skeleton page title is the same font size as title rendered in Page component~~ [new DL][Skeleton Page] Ensure Skeleton Page title is the same font size as title rendered in Page component Oct 9, 2020

francinen force-pushed the skeleton-page-title-fix-font-size branch 4 times, most recently from 9bfa34f to ff48392 Compare October 14, 2020 18:11

francinen marked this pull request as ready for review October 14, 2020 18:11

francinen requested a review from BPScott October 14, 2020 18:11

francinen force-pushed the skeleton-page-title-fix-font-size branch 2 times, most recently from 1fc6369 to 3a4adb8 Compare October 14, 2020 18:17

francinen requested a review from alex-page October 14, 2020 18:17

BPScott reviewed Oct 14, 2020

View reviewed changes

francinen force-pushed the skeleton-page-title-fix-font-size branch from 3a4adb8 to 53cb7f8 Compare October 16, 2020 17:35

francinen requested a review from BPScott October 16, 2020 17:35

francinen force-pushed the skeleton-page-title-fix-font-size branch from 53cb7f8 to d0b22b9 Compare October 16, 2020 18:07

alex-page approved these changes Oct 16, 2020

View reviewed changes

kyledurand approved these changes Oct 19, 2020

View reviewed changes

BPScott reviewed Oct 20, 2020

View reviewed changes

francinen force-pushed the skeleton-page-title-fix-font-size branch from d0b22b9 to c716f00 Compare October 26, 2020 14:48

francinen requested a review from BPScott October 26, 2020 14:49

Ensure skeleton page title is the same font size as page component wh…

d09d172

…en new design language is enabled

francinen force-pushed the skeleton-page-title-fix-font-size branch from c716f00 to d09d172 Compare October 26, 2020 15:16

alex-page approved these changes Oct 28, 2020

View reviewed changes

Merge branch 'master' into skeleton-page-title-fix-font-size

0e9e910

BPScott approved these changes Oct 28, 2020

View reviewed changes

alex-page merged commit 0feaf8c into master Oct 28, 2020

alex-page deleted the skeleton-page-title-fix-font-size branch October 28, 2020 20:32

sylvhama pushed a commit that referenced this pull request Mar 26, 2021

Ensure skeleton page title is the same font size as page component wh…

b7af8cd

…en new design language is enabled (#3449) Co-authored-by: Alex Page <alex@alexpage.com.au>

[new DL][Skeleton Page] Ensure Skeleton Page title is the same font size as title rendered in Page component #3449

[new DL][Skeleton Page] Ensure Skeleton Page title is the same font size as title rendered in Page component #3449

Uh oh!

Conversation

francinen commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

Testing in Storybook

Testing against a Shopify Web

🎩 checklist

Uh oh!

github-actions bot commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Files potentially affected (total: 0)

Files potentially affected (total: 1)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Uh oh!

francinen commented Oct 9, 2020

Uh oh!

kyledurand commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francinen commented Oct 9, 2020

Uh oh!

BPScott commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francinen commented Oct 13, 2020

Uh oh!

BPScott commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francinen commented Oct 14, 2020

Uh oh!

BPScott Oct 14, 2020 • 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!

kyledurand Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

francinen Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

BPScott Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

francinen commented Oct 19, 2020

Uh oh!

kyledurand left a comment

Choose a reason for hiding this comment

Uh oh!

BPScott left a comment

Choose a reason for hiding this comment

Uh oh!

BPScott Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

francinen commented Oct 26, 2020

Uh oh!

alex-page left a comment

Choose a reason for hiding this comment

Uh oh!

kyledurand commented Oct 28, 2020

Uh oh!

BPScott commented Oct 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

francinen commented Oct 9, 2020 •

edited

Loading

github-actions bot commented Oct 9, 2020 •

edited

Loading

kyledurand commented Oct 9, 2020 •

edited

Loading

BPScott commented Oct 9, 2020 •

edited

Loading

BPScott commented Oct 13, 2020 •

edited

Loading

BPScott Oct 14, 2020 •

edited

Loading