Skip to content
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.

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

Change Breadcrumbs to accept a single breadcrumb instead of an array #7990

Merged
merged 1 commit into from
Jan 10, 2023

Conversation

itwasmattgregg
Copy link
Contributor

@itwasmattgregg itwasmattgregg commented Jan 5, 2023

WHY are these changes introduced?

Breadcrumbs has only been using a single breadcrumb for a while now. Instead of allowing consumers to pass an array without knowing why only a single link is being used, we should remove the ability to pass an array and make the component easier to understand implicitly.

WHAT is this pull request doing?

UI will not change. The Breadcrumbs component was already only using the last breadcrumb link in the array.

This PR changes the props for breadcrumbs and also the implementation on the Page component.

Migration path

Since this PR removes the breadcrumbs[] prop in favor of breadcrumb all uses of Page, and Breadcrumbs will need to be migrated.

It will look something like this:

//before
const breadcrumbs = [
      {
        content: 'Products',
        url: 'http://test.com',
      },
];
<Page {...mockProps} breadcrumbs={breadcrumbs} />

//after
const breadcrumb = {
  content: 'Products',
  url: 'http://test.com',
};
<Page {...mockProps} breadcrumb={breadcrumb} />

How to 🎩

Check the Breadcrumbs and Page components in storybook

🎩 checklist

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

size-limit report 📦

Path Size
polaris-react-cjs 211.33 KB (-0.01% 🔽)
polaris-react-esm 136.47 KB (-0.02% 🔽)
polaris-react-esnext 191.69 KB (-0.02% 🔽)
polaris-react-css 41.76 KB (0%)

@itwasmattgregg
Copy link
Contributor Author

Not sure how to fix the linting errors.

@kyledurand
Copy link
Member

Not sure how to fix the linting errors.

/home/runner/work/polaris/polaris/polaris-react/src/components/Collapsible/Collapsible.tsx
@shopify/polaris:lint: 26:3 error 'preventMeasuringOnChildrenUpdate

This probably snuck back into this branch from me updating earlier today.

If you update v11-major locally and rebase this branch against it then push it back up you should be good

@alex-page
Copy link
Member

@itwasmattgregg would you consider creating a migration for this work? We have a migration package and it would help Polaris developers onboard to the breaking change.

@itwasmattgregg
Copy link
Contributor Author

@itwasmattgregg would you consider creating a migration for this work? We have a migration package and it would help Polaris developers onboard to the breaking change.

I can try for sure. Haven't written one before. Is that a separate PR? @alex-page

@alex-page
Copy link
Member

@itwasmattgregg I would make another PR. We have a bunch of documentation/examples here https://github.com/Shopify/polaris/tree/main/polaris-migrator

@BPScott
Copy link
Member

BPScott commented Jan 9, 2023

When introducing a breaking change, aim to introduce the new approach in minor version, so that people can gradually adopt the new approach rather than it having perform the migration to the new approach in the major update PR.

What do you think to splitting this into two PRs:

  • A PR against the main branch that allows consumers to specify breadcrumbs as an array or a single object
  • A PR against the v11 branch that removes the ability to specify breadcrumbs as an array.

Doing so would allow people to start using the single-object style immediately, and we can transition web to that style, prior to the v11 PR (whenever that appears)

@itwasmattgregg
Copy link
Contributor Author

When introducing a breaking change, aim to introduce the new approach in minor version, so that people can gradually adopt the new approach rather than it having perform the migration to the new approach in the major update PR.

What do you think to splitting this into two PRs:

  • A PR against the main branch that allows consumers to specify breadcrumbs as an array or a single object
  • A PR against the v11 branch that removes the ability to specify breadcrumbs as an array.

Doing so would allow people to start using the single-object style immediately, and we can transition web to that style, prior to the v11 PR (whenever that appears)

There's not really a way to mark the array version as "deprecated" is there.

@alex-page
Copy link
Member

alex-page commented Jan 10, 2023

There's not really a way to mark the array version as "deprecated" is there.

No need to mark a prop type as deprecated. This would make moving a big repo like shopify/web across to this change easier. We can do the heavy lifting without it all being in a giant PR of breaking changes.

@BPScott
Copy link
Member

BPScott commented Jan 10, 2023

There's not really a way to mark the array version as "deprecated" is there.

Not that I can think of. You can mark a whole prop as deprecated,however adding a new prop with a new name and removing the old prop feels like overkill here. I think no deprecation notice for this is fine.

@itwasmattgregg itwasmattgregg merged commit 590711c into v11-major Jan 10, 2023
@itwasmattgregg itwasmattgregg deleted the breadcrumbs-to-breadcrumb branch January 10, 2023 18:07
itwasmattgregg added a commit that referenced this pull request Feb 23, 2023
…Breadcrumbs (#8135)

### WHY are these changes introduced?

As a followup to #8016 in
preparation for v11 where breadcrumb becomes singular
(#7990).

I should have originally done this and deprecated breadcrumbs but at the
time we had a different upgrade path in mind. After collaborating with a
number of folks we determined deprecating breadcrumbs officially and
moving to a singular breadcrumb in a `backAction` prop for v11 was the
best path.

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

<!-- ℹ️ 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)

<!--
  Give as much information as needed to experiment with the component
  in the playground.
-->

<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

- [ ] 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

---------

Co-authored-by: Chloe Rice <chloerice@users.noreply.github.com>
sam-b-rose added a commit that referenced this pull request May 26, 2023
## @shopify/polaris v11.0.0

### Dependencies

- [x] #8200

### NodeJS

- [x] #8201

### TypeScript

- [x] #8203

### Components

- [x] #7349
- [x] #7397
- [x] #7962
- [x] #8187
- [x] #8184
- [x] #8206
- [x] #7990
- [x] #8468
- [x] #8577
- [x] #8631
- [x] #8962

## @shopify/polaris-tokens v7.0.0

### Tokens
- [x] #6920
- [x] #8245
- [x] #4826
- [x] #8405

## @shopify/stylelint-polaris v7.0.0
- [x] #7622
- [x] #8419

# Post @shopify/polaris v11 shipping
- [ ] #8420

## Low priority or not ready breaking changes
- [x] Remove deprecated layout components
- [x] Release Layout primitive components

---------

Co-authored-by: Tim Layton <tmlayton@users.noreply.github.com>
Co-authored-by: Ryan Musgrave <ryan.musgrave@shopify.com>
Co-authored-by: Ryan Musgrave <ryanm128@gmail.com>
Co-authored-by: aveline <aveline@users.noreply.github.com>
Co-authored-by: Kyle Durand <kyledurand@users.noreply.github.com>
Co-authored-by: Matt Gregg <matt.gregg@shopify.com>
Co-authored-by: Alex Page <hi@alexpage.dev>
Co-authored-by: Lo Kim <lo.kim@shopify.com>
Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com>
Co-authored-by: Aaron Casanova <32409546+aaronccasanova@users.noreply.github.com>
Co-authored-by: Sam Rose <11774595+samrose3@users.noreply.github.com>
Co-authored-by: Sam Rose <sam.rose@shopify.com>
Co-authored-by: Marc Thomas <marc.thomas@shopify.com>
Co-authored-by: Alex Page <19199063+alex-page@users.noreply.github.com>
Co-authored-by: Chloe Rice <18447883+chloerice@users.noreply.github.com>
Co-authored-by: Chloe Rice <chloerice@users.noreply.github.com>
Co-authored-by: Joe Thomas <joe.thomas@shopify.com>
Co-authored-by: Yuraima Estevez <yuraima.estevez@shopify.com>
Co-authored-by: shopify-github-actions-access[bot] <109624739+shopify-github-actions-access[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
juzser pushed a commit to juzser/polaris that referenced this pull request Jul 27, 2023
…Breadcrumbs (Shopify#8135)

### WHY are these changes introduced?

As a followup to Shopify#8016 in
preparation for v11 where breadcrumb becomes singular
(Shopify#7990).

I should have originally done this and deprecated breadcrumbs but at the
time we had a different upgrade path in mind. After collaborating with a
number of folks we determined deprecating breadcrumbs officially and
moving to a singular breadcrumb in a `backAction` prop for v11 was the
best path.

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

<!-- ℹ️ 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)

<!--
  Give as much information as needed to experiment with the component
  in the playground.
-->

<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

- [ ] 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

---------

Co-authored-by: Chloe Rice <chloerice@users.noreply.github.com>
juzser pushed a commit to juzser/polaris that referenced this pull request Jul 27, 2023
## @shopify/polaris v11.0.0

### Dependencies

- [x] Shopify#8200

### NodeJS

- [x] Shopify#8201

### TypeScript

- [x] Shopify#8203

### Components

- [x] Shopify#7349
- [x] Shopify#7397
- [x] Shopify#7962
- [x] Shopify#8187
- [x] Shopify#8184
- [x] Shopify#8206
- [x] Shopify#7990
- [x] Shopify#8468
- [x] Shopify#8577
- [x] Shopify#8631
- [x] Shopify#8962

## @shopify/polaris-tokens v7.0.0

### Tokens
- [x] Shopify#6920
- [x] Shopify#8245
- [x] Shopify#4826
- [x] Shopify#8405

## @shopify/stylelint-polaris v7.0.0
- [x] Shopify#7622
- [x] Shopify#8419

# Post @shopify/polaris v11 shipping
- [ ] Shopify#8420

## Low priority or not ready breaking changes
- [x] Remove deprecated layout components
- [x] Release Layout primitive components

---------

Co-authored-by: Tim Layton <tmlayton@users.noreply.github.com>
Co-authored-by: Ryan Musgrave <ryan.musgrave@shopify.com>
Co-authored-by: Ryan Musgrave <ryanm128@gmail.com>
Co-authored-by: aveline <aveline@users.noreply.github.com>
Co-authored-by: Kyle Durand <kyledurand@users.noreply.github.com>
Co-authored-by: Matt Gregg <matt.gregg@shopify.com>
Co-authored-by: Alex Page <hi@alexpage.dev>
Co-authored-by: Lo Kim <lo.kim@shopify.com>
Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com>
Co-authored-by: Aaron Casanova <32409546+aaronccasanova@users.noreply.github.com>
Co-authored-by: Sam Rose <11774595+samrose3@users.noreply.github.com>
Co-authored-by: Sam Rose <sam.rose@shopify.com>
Co-authored-by: Marc Thomas <marc.thomas@shopify.com>
Co-authored-by: Alex Page <19199063+alex-page@users.noreply.github.com>
Co-authored-by: Chloe Rice <18447883+chloerice@users.noreply.github.com>
Co-authored-by: Chloe Rice <chloerice@users.noreply.github.com>
Co-authored-by: Joe Thomas <joe.thomas@shopify.com>
Co-authored-by: Yuraima Estevez <yuraima.estevez@shopify.com>
Co-authored-by: shopify-github-actions-access[bot] <109624739+shopify-github-actions-access[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants