Skip to content
This repository was archived by the owner on Sep 30, 2025. It is now read-only.

Conversation

mattkubej
Copy link
Contributor

@mattkubej mattkubej commented Sep 18, 2023

WHY are these changes introduced?

Fixes https://github.com/Shopify/web/issues/103405

Inconsistent Header height within the Page component is leading to the Header shifting vertically when navigating between pages with different properties enabled (e.g. metadata, actions). This pull request makes adjustments to combat the shifting.

WHAT is this pull request doing?

  • Consistent Header row height of 28px
    • Header Title will always have 2px of vertical padding to bring the 24px line height to 28px in total for the container
    • Allow for bleeding of the Title metadata, so metadata can be larger than Title without impacting alignment (e.g. Location picker)
  • Removes vertical alignment and margin-top of Title metadata, which had no impact to aesthetic
  • Reorganizes CSS to make class/style relationships clearer
  • Adds new Storybook to highlight problem

NOTE: This is a bit of a bandaid fix. We should likely rewrite this component to utilize a grid layout to have a more resilient solution for achieving the desired aesthetic, but considering the variability in configurations of this component that is a sizable effort. However, I think we'd need to understand the desired aesthetic when a subtitle exists along with title metadata content that is larger than the title to determine the optimal solution.

How to 🎩

Spin - Web
Spin - Polaris Storybook

Validate when navigating amongst root pages (e.g. draft orders, products) that the Page Header does not shift with/without the presence of title metadata and actions. Also, validate that the Header row has contents vertically centered.

🎩 checklist

@mattkubej mattkubej force-pushed the page/header-vertical-alignment branch from 4368825 to d2ad246 Compare September 18, 2023 23:15
@mattkubej
Copy link
Contributor Author

/snapit

@github-actions
Copy link
Contributor

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

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

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20230918232254
yarn add @shopify/polaris@0.0.0-snapshot-release-20230918232254
yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20230918232254
yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20230918232254

sophschneider and others added 3 commits September 29, 2023 16:00
### WHY are these changes introduced?

The noScroll prop on our modal should stop the scroll effect. This would
be useful in cases where scrolls are already in the children components.


https://github.com/Shopify/polaris/assets/113911518/ba755e78-851a-4809-b3a3-5e31a8d7a7db

We are using this throughout Shopify on our resource pickers where
noScroll is set to true. The issue is right now Banner is scrolling and
causing a double scroll


https://github.com/Shopify/polaris/assets/113911518/ddaf01b6-1592-4a34-a36d-6e4e0e9b6f2c


### WHAT is this pull request doing?

We are simply adding the `overflow-y: hidden`.

`overflow-x: hidden` is already present but that only stops horizontal
scroll

### How to 🎩

Add and remove the onScroll props to see scroll ability disappear. (You
might need to shorten your window to get the scroll if your text isn't
long enough)

<details>
<summary>Copy-paste this code in
<code>playground/Playground.tsx</code>:</summary>

```jsx
import React, {useState, useCallback} from 'react';

import {Page, Modal, Button} from '../src';

export function Playground() {
  const [active, setActive] = useState(true);

  const handleChange = useCallback(() => setActive(!active), [active]);

  const activator = <Button onClick={handleChange}>Open</Button>;

  return (
    <Page title="Playground">
      {activator}
      <Modal
        activator={activator}
        title="Reach more shoppers with Instagram product tags"
        open={active}
        onClose={handleChange}
        noScroll
      >
        <p>
          Use Instagram posts to share your products with millions of people.
          Let shoppers buy from your store without leaving Instagram.
        </p>
        <p>
          Use Instagram posts to share your products with millions of people.
          Let shoppers buy from your store without leaving Instagram.
        </p>
        <p>
          Use Instagram posts to share your products with millions of people.
          Let shoppers buy from your store without leaving Instagram.
        </p>
        <p>
          Use Instagram posts to share your products with millions of people.
          Let shoppers buy from your store without leaving Instagram.
        </p>
        <p>
          Use Instagram posts to share your products with millions of people.
          Let shoppers buy from your store without leaving Instagram.
        </p>
        <p>
          Use Instagram posts to share your products with millions of people.
          Let shoppers buy from your store without leaving Instagram.
        </p>
        <p>
          Use Instagram posts to share your products with millions of people.
          Let shoppers buy from your store without leaving Instagram.
        </p>
        <p>
          Use Instagram posts to share your products with millions of people.
          Let shoppers buy from your store without leaving Instagram.
        </p>
      </Modal>
    </Page>
  );
}
```

</details>

### 🎩 checklist

- [x] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [x] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)

Co-authored-by: kellydanma <kelly.ma@mail.mcgill.ca>
Revert for making #10580 part of
v12.x.x

### WHY are these changes introduced?

Closes https://github.com/Shopify/core-issues/issues/60374 

We believe giving users more visibility to the move column actions will
enhance their experience.

### WHAT is this pull request doing?

<!--
  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>
-->

1. Create an `EditColumnsButton` that handles the markup for the edit
column action.
2. Add a new property to `<IndexFilters/>` called `showEditColumns`,
that when is true, renders the `<EditColumnsButton/>`
3. Add storybook and test implementations. 



https://github.com/Shopify/polaris/assets/5873627/5368e55f-839d-48eb-9362-dbe7d0e1a098

**Updated order or buttons:**

![Screenshot 2023-09-20 at 14 05
28](https://github.com/Shopify/polaris/assets/5873627/4ada4c7a-4182-469b-b506-aea01432f170)


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

### How to 🎩

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development)
🗒 [General tophatting
guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md)
📄 [Changelog
guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog)

1. Run storybook. 
2. Go to IndexFilters > With Edit Columns Button. 
3. Play with the new button ✨ 

<details>
<summary>Copy-paste this code in
<code>playground/Playground.tsx</code>:</summary>

```jsx
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}
```

</details>

### 🎩 checklist

- [x] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [x] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [x] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [x] Updated the component's `README.md` with documentation changes
- [x] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide
@mattkubej mattkubej force-pushed the page/header-vertical-alignment branch from d2ad246 to 3906a7d Compare October 2, 2023 23:47
@mattkubej mattkubej requested review from a team as code owners October 2, 2023 23:47
@mattkubej mattkubej force-pushed the page/header-vertical-alignment branch from a7f163d to 195296c Compare October 2, 2023 23:51
@mattkubej
Copy link
Contributor Author

/snapit

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023

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

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

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20231002235641
yarn add @shopify/polaris@0.0.0-snapshot-release-20231002235641

Copy link
Contributor

@mrcthms mrcthms left a comment

Choose a reason for hiding this comment

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

Looks good, nice work!

Copy link
Member

@sam-b-rose sam-b-rose left a comment

Choose a reason for hiding this comment

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

🎩 looks good! Can we target the 12.x.x branch? We are tee'ing up the next major release this week and v12 is likely the next version be be shipped.

Also fine with having this fix in main but we'll also want to resolve any conflicts and ensure these changes are properly carried over to 12.x.x

Copy link

@ouellettejordan ouellettejordan left a comment

Choose a reason for hiding this comment

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

Tested spin link and looks good to me 👍

@mattkubej
Copy link
Contributor Author

mattkubej commented Oct 3, 2023

🎩 looks good! Can we target the 12.x.x branch? We are tee'ing up the next major release this week and v12 is likely the next version be be shipped.

Yea, I can do that, but I won't be able to get to that until this evening if that's alright @sam-b-rose

Adds support for `@custom-media` using the
[postcss-global-data](https://github.com/csstools/postcss-plugins/blob/main/plugins/postcss-global-data/README.md)
and
[postcss-custom-media](https://github.com/csstools/postcss-plugins/blob/main/plugins/postcss-custom-media/README.md)
plugins.

See `Text` component example of how this will migrate our breakpoints
away from the current SCSS method:

<img width="630" alt="Screenshot 2023-10-02 at 3 05 03 PM"
src="https://github.com/Shopify/polaris/assets/11774595/613b1f32-eb96-4a14-8dc8-188536493efe">
@mattkubej mattkubej changed the base branch from main to v12.x.x October 11, 2023 01:47
@mattkubej mattkubej changed the base branch from v12.x.x to main October 11, 2023 01:48
@mattkubej mattkubej force-pushed the page/header-vertical-alignment branch from 195296c to 60e65c1 Compare October 11, 2023 01:54
@mattkubej mattkubej changed the base branch from main to v12.x.x October 11, 2023 01:54
@mattkubej mattkubej force-pushed the page/header-vertical-alignment branch from 60e65c1 to d9bf2a8 Compare October 11, 2023 01:57
@mattkubej
Copy link
Contributor Author

/snapit

@mattkubej mattkubej force-pushed the page/header-vertical-alignment branch 2 times, most recently from eeef22b to 46c125d Compare October 11, 2023 02:51
@mattkubej
Copy link
Contributor Author

/snapit

sam-b-rose and others added 2 commits October 11, 2023 09:12
Run `yarn changeset pre exit` to exit prerelease mode.

This was also [done in
`main`](560897e)
before releasing v12.

Snapshots won't work when in prerelease mode. We can now exit prerelease
since v12 has been officially released.

See the [Changesets Prerelease
documentation](https://github.com/changesets/changesets/blob/main/docs/prereleases.md)
for more details.
@mattkubej mattkubej force-pushed the page/header-vertical-alignment branch from 46c125d to ef49094 Compare October 11, 2023 13:35
@sam-b-rose
Copy link
Member

/snapit

@github-actions
Copy link
Contributor

🫰✨ Thanks @sam-b-rose! Your snapshots have been published to npm.

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

yarn add @shopify/polaris@0.0.0-snapshot-release-20231011140942
yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20231011140942
yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20231011140942

@mattkubej
Copy link
Contributor Author

/snapit

@github-actions
Copy link
Contributor

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

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

yarn add @shopify/polaris@0.0.0-snapshot-release-20231011162506
yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20231011162506
yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20231011162506

@mattkubej
Copy link
Contributor Author

Closing in favor of #11168 due to the v12 changes

@mattkubej mattkubej closed this Nov 14, 2023
SMAKSS pushed a commit to SMAKSS/polaris that referenced this pull request Nov 26, 2023
…hopify#11168)

### WHY are these changes introduced?

Refresh of Shopify#10545 post v12

Fixes: Shopify/web#103405

Inconsistent Header height within the `Page` component is leading to the
Header shifting vertically when navigating between pages with different
properties enabled (e.g. metadata, actions). This pull request makes
adjustments to combat the shifting.

### WHAT is this pull request doing?

- Consistent Header row height of 28px
- Header Title will always have 2px of vertical padding to bring the
24px line height to 28px in total for the container
- Allow for bleeding of the Title metadata, so metadata can be larger
than Title without impacting alignment (e.g. Location picker)
- Removes vertical alignment and margin-top of Title metadata, which had
no impact to aesthetic
- Reorganizes CSS to make class/style relationships clearer
- Adds new Storybook to highlight problem

NOTE: This is a bit of a bandaid fix. We should likely rewrite this
component to utilize a grid layout to have a more resilient solution for
achieving the desired aesthetic, but considering the variability in
configurations of this component that is a sizable effort. However, I
think we'd need to understand the desired aesthetic when a subtitle
exists along with title metadata content that is larger than the title
to determine the optimal solution.

### How to 🎩

[Spin -
Web](https://admin.web.polaris-page-header-position.matt-kubej.us.spin.dev/store/shop1/orders?inContextTimeframe=none)
[Spin - Polaris
Storybook](https://storybook.web.polaris-page-header-position.matt-kubej.us.spin.dev/?path=/story/all-components-page--with-content-after-title-and-subtitle)

Validate when navigating amongst root pages (e.g. draft orders,
products) that the Page Header does not shift with/without the presence
of title metadata and actions. Also, validate that the Header row has
contents vertically centered.

### 🎩 checklist

- [x] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [x] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [x] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [x] Updated the component's `README.md` with documentation changes
- [x] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide
ascherkus pushed a commit to ascherkus/polaris that referenced this pull request Feb 19, 2025
…hopify#11168)

### WHY are these changes introduced?

Refresh of Shopify#10545 post v12

Fixes: Shopify/web#103405

Inconsistent Header height within the `Page` component is leading to the
Header shifting vertically when navigating between pages with different
properties enabled (e.g. metadata, actions). This pull request makes
adjustments to combat the shifting.

### WHAT is this pull request doing?

- Consistent Header row height of 28px
- Header Title will always have 2px of vertical padding to bring the
24px line height to 28px in total for the container
- Allow for bleeding of the Title metadata, so metadata can be larger
than Title without impacting alignment (e.g. Location picker)
- Removes vertical alignment and margin-top of Title metadata, which had
no impact to aesthetic
- Reorganizes CSS to make class/style relationships clearer
- Adds new Storybook to highlight problem

NOTE: This is a bit of a bandaid fix. We should likely rewrite this
component to utilize a grid layout to have a more resilient solution for
achieving the desired aesthetic, but considering the variability in
configurations of this component that is a sizable effort. However, I
think we'd need to understand the desired aesthetic when a subtitle
exists along with title metadata content that is larger than the title
to determine the optimal solution.

### How to 🎩

[Spin -
Web](https://admin.web.polaris-page-header-position.matt-kubej.us.spin.dev/store/shop1/orders?inContextTimeframe=none)
[Spin - Polaris
Storybook](https://storybook.web.polaris-page-header-position.matt-kubej.us.spin.dev/?path=/story/all-components-page--with-content-after-title-and-subtitle)

Validate when navigating amongst root pages (e.g. draft orders,
products) that the Page Header does not shift with/without the presence
of title metadata and actions. Also, validate that the Header row has
contents vertically centered.

### 🎩 checklist

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

Successfully merging this pull request may close these issues.

8 participants