Skip to content

[Layout foundations] Update SkeletonPage title to have fixed height #8075

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 19, 2023
Merged

[Layout foundations] Update SkeletonPage title to have fixed height #8075

merged 1 commit into from
Jan 19, 2023

Conversation

@chazdean
Copy link
Contributor

@chazdean chazdean commented Jan 18, 2023 •
edited
Loading

WHY are these changes introduced?

Adds consistency to SkeletonPage by giving the skeleton title a fixed height regardless if the primaryAction prop is being used or not

WHAT is this pull request doing?

  • Centers the title skeleton by adding blockStart and blockEnd padding
  • Increases title height back to 28px so that with 8px of extra padding, title container can match the height of the primaryAction skeleton at 36px

With primaryAction skeleton

Screenshot 2023-01-18 at 8 37 53 AM

Without primaryAction skeleton

Screenshot 2023-01-18 at 8 38 50 AM

How to 🎩

Spin Link (Updated)

🎩 checklist

@chazdean chazdean self-assigned this Jan 18, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 18, 2023 •
edited
Loading

size-limit report 📦

Path Size
polaris-react-cjs 214.79 KB (+0.01% 🔺)
polaris-react-esm 137.23 KB (+0.01% 🔺)
polaris-react-esnext 191.53 KB (+0.01% 🔺)
polaris-react-css 41 KB (0%)

@chazdean
Copy link
Contributor Author

chazdean commented Jan 18, 2023

/snapit

@github-actions
Copy link
Contributor

github-actions bot commented Jan 18, 2023

🫰✨ Thanks @chazdean! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20230118021423
yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20230118021423
yarn add @shopify/polaris@0.0.0-snapshot-release-20230118021423

@chazdean chazdean marked this pull request as ready for review January 18, 2023 13:44
@kyledurand
Copy link
Member

kyledurand commented Jan 18, 2023

This horizontal alignment looks off. Is that intentional?

Before After
image image

@chazdean
Copy link
Contributor Author

chazdean commented Jan 18, 2023 •
edited
Loading

This horizontal alignment looks off. Is that intentional?

Before After
image image

@kyledurand Yeah the alignment definitely looks off. There looks to be padding-left on the .HeaderTitleWrapper selector in ResourceList.scss that is no longer needed. I created a separate issue for this 👍🏾

@chazdean chazdean mentioned this pull request Jan 18, 2023
Closed
@sarahill
Copy link
Contributor

sarahill commented Jan 18, 2023 •
edited
Loading

Seeing some horizontal misalignment on the Customers index page

  • I'm assuming this is because there's a picker in the page header. I'm guessing the same issue will occur on the order index when there are multiple locations to choose from.
  • It's also loosing left/right padding at smaller breakpoints

image

image

@chazdean
Copy link
Contributor Author

chazdean commented Jan 19, 2023 •
edited
Loading

Seeing some horizontal misalignment on the Customers index page

  • I'm assuming this is because there's a picker in the page header. I'm guessing the same issue will occur on the order index when there are multiple locations to choose from.
  • It's also loosing left/right padding at smaller breakpoints

@sarahill This issue is present in production as well. Here a custom css skeleton title header is being used causing these differences which will have to be revisited 😕. The orders index page should be good though as it will use our new rebuilt SkeletonPage header regardless if there are multiple locations to choose from 👍🏾

@chazdean chazdean mentioned this pull request Jan 19, 2023
Merged
5 tasks
@kyledurand
Copy link
Member

kyledurand commented Jan 19, 2023

Just merged @aveline's PR, can you rebase against batch 2 and update spin @chazdean?

@chazdean chazdean force-pushed the update-skeleton-page-header branch from 6f2906d to c276c82 Compare January 19, 2023 15:05
@chazdean
Copy link
Contributor Author

chazdean commented Jan 19, 2023

/snapit

@github-actions
Copy link
Contributor

github-actions bot commented Jan 19, 2023

🫰✨ Thanks @chazdean! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20230119151020
yarn add @shopify/polaris-icons@0.0.0-snapshot-release-20230119151020
yarn add @shopify/polaris@0.0.0-snapshot-release-20230119151020

@chazdean
Copy link
Contributor Author

chazdean commented Jan 19, 2023

@kyledurand good to go 👍🏾

yurm04
yurm04 approved these changes Jan 19, 2023
View reviewed changes
Copy link
Contributor

@yurm04 yurm04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are some chromatic diffs that look a bit wonky that might just be the same issue we've been dealing with throughout the batches. Code LGTM, maybe needs one more pass in chromatic for UX review 👍

aveline
aveline approved these changes Jan 19, 2023
View reviewed changes
Copy link
Contributor

@aveline aveline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chromatic changes need to be reviewed, the change in title heights make sense and need to be accepted, but not sure what's going on with the centering. Otherwise looks good 👍

sarahill
sarahill approved these changes Jan 19, 2023
View reviewed changes
Copy link
Contributor

@sarahill sarahill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of the chromatic diffs look off but look and function as expected in Storybook ✅

@chazdean chazdean merged commit d5196fe into layout-rebuild-batch-2 Jan 19, 2023
@chazdean chazdean deleted the update-skeleton-page-header branch January 19, 2023 16:23
laurkim pushed a commit that referenced this pull request Jan 26, 2023
@chazdean @laurkim
[Layout foundations] Update SkeletonPage title to have fixed height (…
9abbc84 
…#8075)

<!--
  ☝️How to write a good PR title:
- Prefix it with [ComponentName] (if applicable), for example: [Button]
  - Start with a verb, for example: Add, Delete, Improve, Fix…
  - Give as much context as necessary and as little as possible
  - Prefix it with [WIP] while it’s a work in progress
-->

### WHY are these changes introduced?

Adds consistency to `SkeletonPage` by giving the skeleton title a fixed
height regardless if the `primaryAction` prop is being used or not

<!--
  Context about the problem that’s being addressed.
-->

### WHAT is this pull request doing?

- Centers the title skeleton by adding `blockStart` and `blockEnd`
padding
- Increases title height back to `28px` so that with `8px` of extra
padding, title container can match the height of the `primaryAction`
skeleton at `36px`

#### With primaryAction skeleton
<img width="1169" alt="Screenshot 2023-01-18 at 8 37 53 AM"
src="https://user-images.githubusercontent.com/59836805/213186715-c011be36-398b-4b8e-8172-b842277cc50d.png">

#### Without primaryAction skeleton
<img width="1171" alt="Screenshot 2023-01-18 at 8 38 50 AM"
src="https://user-images.githubusercontent.com/59836805/213186743-1b3c6d38-4680-458f-a5c3-87206ee89914.png">


<!--
  Summary of the changes committed.

Before / after screenshots are appreciated for UI changes. Make sure to
include alt text that describes the screenshot.

If you include an animated gif showing your change, wrapping it in a
details tag is recommended. Gifs usually autoplay, which can cause
accessibility issues for people reviewing your PR:

    <details>
      <summary>Summary of your gif(s)</summary>
      <img src="..." alt="Description of what the gif shows">
    </details>
-->

<!-- ℹ️ Delete the following for small / trivial changes -->

### How to 🎩

[Spin
Link](https://admin.web.web-6n2m.chaz-dean.us.spin.dev/store/shop1/)
(Updated)

</details>

### 🎩 checklist

- [ ] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [ ] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [ ] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [ ] Updated the component's `README.md` with documentation changes
- [ ] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sarahill sarahill sarahill approved these changes

@kyledurand kyledurand Awaiting requested review from kyledurand

@nayeobkim nayeobkim Awaiting requested review from nayeobkim

@SeanyB SeanyB Awaiting requested review from SeanyB

+2 more reviewers

@aveline aveline aveline approved these changes

@yurm04 yurm04 yurm04 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@chazdean chazdean

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@chazdean @kyledurand @sarahill @aveline @yurm04