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

[Sheet] Deprecate component #4210

Merged
merged 7 commits into from
May 19, 2021
Merged

[Sheet] Deprecate component #4210

merged 7 commits into from
May 19, 2021

Conversation

martenbjork
Copy link
Contributor

@martenbjork martenbjork commented May 17, 2021

This PR deprecates the sheet component.

WHY are these changes introduced?

  • The sheet component makes it easy to ship a bad experience. It encourages designers to create a new layer on top of the page instead of solving the hard problem — integrating the new UI into the existing one. There are cases where a sheet component could make sense, but on a system level, it results in more bad UIs than good ones.
  • Sheets block other parts of the UI
  • Sheets force you to switch context, which adds cognitive load
  • You can’t link into a sheet (you could make it happen technically, but it would be awkward)
  • It adds 2 clicks to any interaction

Most of this is true for modals as well. In general, we should avoid modal patterns as much as possible.

WHAT is this pull request doing?

Example of how this will show up in the style guide: (For exact wording, see the "files changed" tab.)

image

Example of how this will show up in the Figma library:

Screen Shot 2021-05-19 at 11 30 26 AM

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
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>
  );
}

🎩 checklist

  • 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

@martenbjork martenbjork changed the title [WIP] Deprecate Sheet component Deprecate Sheet component May 18, 2021
@martenbjork martenbjork changed the title Deprecate Sheet component [Sheet] Deprecate component May 18, 2021
@github-actions
Copy link
Contributor

size-limit report

Path Size
cjs 141.84 KB (0%)
esm 95.44 KB (0%)
esnext 138.7 KB (0%)
css 33.77 KB (0%)

@martenbjork martenbjork marked this pull request as ready for review May 18, 2021 19:01
@alex-page alex-page requested a review from kyledurand May 19, 2021 17:26
src/components/Sheet/README.md Outdated Show resolved Hide resolved
Co-authored-by: Sadie Redden <sadie.redden@shopify.com>
Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

👍

@martenbjork martenbjork merged commit d0e0477 into main May 19, 2021
@martenbjork martenbjork deleted the deprecate-sheets branch May 19, 2021 19:01
@tobi
Copy link
Member

tobi commented May 19, 2021

I'm a big fan of subtraction. I randomly heard about this ticket, and really comment the team for doing this. It takes courage to subtract things but its necessary for building focused and great systems like Polaris.

@mbaumbach
Copy link
Contributor

I think this is a good deprecation. Is there any published guidance on what is going to happen to the Filters component, which makes use of the Sheet?

We have similar implementations that followed this design pattern and are always interested in leveraging the great minds on the Polaris design team for inspiration.

@alex-page
Copy link
Member

@mbaumbach as of now the filters will remain using the sheet. On the next major release we will move them into the Filters component so the pattern is not accessible to solve multiple problems.

Removing the sheet pattern for filters would require an understanding on how users filter across the entire admin experience. If you are looking for a UX pattern that doesn't use the sheet I would recommend PC Part Picker. A change like this in the admin would require changes to the admin frame (header, sidebar) to create more space.

@StefanNeuser
Copy link

I'm a big fan of subtraction. I randomly heard about this ticket, and really comment the team for doing this. It takes courage to subtract things but its necessary for building focused and great systems like Polaris.

We are constantly following Polaris because we always want to be up to date with our apps. They are doing are really good job, keep going!

PS: The last Unite was world class! Best regards from Koblenz, 247APPS Team :-)

@paulgrieselhuber
Copy link

@mbaumbach as of now the filters will remain using the sheet. On the next major release we will move them into the Filters component so the pattern is not accessible to solve multiple problems.

Removing the sheet pattern for filters would require an understanding on how users filter across the entire admin experience. If you are looking for a UX pattern that doesn't use the sheet I would recommend PC Part Picker. A change like this in the admin would require changes to the admin frame (header, sidebar) to create more space.

Our use case for sheets involves using them for on-page documentation (i.e. a quick pop-out for more details). It is particularly useful because the international audience for the application is not very technically sophisticated, and having these reminders and instructions easily available at any part of the interface is really quite helpful for them.

Losing the sheet component actually creates more of a context change for us, which I understand is the opposite of the objective with this deprecation: the user has to go to a completely different page, with likely a different layout (for example, a wiki).

The idea that the only valid use case for Sheet is the way you use it in the Filter component (which, it sounds like you will be maintaining, by the way) seems a bit...not fully considered. I appreciate that you will be maintaining it for Filters, because we are also starting to use Sheet in this way for that component as well.

Could you please reconsider deprecating this component?

@chaddjohnson
Copy link

chaddjohnson commented Sep 26, 2021

I think there are valid uses cases for Sheet. I just built an app that lets someone design templates, and a Sheet is used to view and edit properties for the template. The properties do not need to always be visible, and they need to be visible, when desired, alongside the template. A modal would be inappropriate as it would cover the template.

Please, please don't get rid of this component. Sheet has been a really good solution to meet my app's needs.

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.

None yet

9 participants