[Modal] Add prop small to decrease Modal's width #4177

thiagoloschi · 2021-05-06T18:18:23Z

WHY are these changes introduced?

We'd like to be able to decrease Modals' width similarly to how we're currently able to increase it through the prop large.

WHAT is this pull request doing?

This PR adds a prop small to the Modal component so that users are able to decrease Modals' width in a similar manner that they can increase it through the prop large.

We've considered adding a prop width that could receive either small | medium(default) | large, which would avoid having two props that control width being passed into the component simultaneously, but that would break the current API. Therefore, if both small and large are passed in at the same time, we'll default to large.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Another example with the code from README.md

Copy-paste this code in playground/Playground.tsx:

To see this addition live, just add a small prop to the Modal component. Please make sure the prop large is not being passed simultaneously, as it would take priority over the small prop.

import React, {useState} from 'react';
import {Page, Modal, Button, TextContainer} from '../src';

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

  const handleChange = () => setActive(!active);

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

  return (
    <Page title="Playground">
      <Modal
        activator={activator}
        open={active}
        onClose={handleChange}
        title="Reach more shoppers with Instagram"
        small
        primaryAction={{
          content: 'Add Instagram',
          onAction: handleChange,
        }}
        secondaryActions={[
          {
            content: 'Learn more',
            onAction: handleChange,
          },
        ]}
      >
        <Modal.Section>
          <TextContainer>
            <p>
              Use Instagram posts to share your products with millions of
              people. Let shoppers buy from your store without leaving
              Instagram.
            </p>
          </TextContainer>
        </Modal.Section>
      </Modal>
    </Page>
  );
}

🎩 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

ghost · 2021-05-06T18:18:27Z

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

github-actions · 2021-05-06T18:20:47Z

🟡 This pull request modifies 6 files and might impact 5 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:

All files potentially affected (total: 5)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Modal/Modal.tsx (total: 3)

Files potentially affected (total: 3)

📄 src/components/Modal/README.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/Modal/components/Dialog/Dialog.scss (total: 5)

Files potentially affected (total: 5)

🧩 src/components/Modal/components/Dialog/Dialog.tsx (total: 4)

Files potentially affected (total: 4)

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

Files potentially affected (total: 0)

thiagoloschi · 2021-05-06T20:45:39Z

src/components/Modal/components/Dialog/Dialog.scss

+$minimal-width: rem(380px);
 $small-width: rem(620px);
 $large-width: rem(980px);


I wasn't sure if I could rename this, but ideally, we should have small | medium | large tokens instead.

thiagoloschi · 2021-05-06T20:48:03Z

src/components/Modal/README.md

+        </Modal.Section>
+      </Modal>
+    </div>
+  );


It should render this:

thiagoloschi · 2021-05-06T20:50:31Z

src/components/Modal/Modal.tsx

  /** Increases the modal width */
  large?: boolean;
+  /** Decreases the modal width */
+  small?: boolean;


I wonder if we should add a prop width that takes in small | medium (default) | large and keep the prop large until we're good to deprecate it in future versions. I think that makes more sense because then we would avoid having these two props (small and large) being passed simultaneously.

I agree. I think it makes more sense to have a width prop rather than small & large!

We've seen similar API bloat with the Button component. It has > 10 modifiers and depending on which ones you apply. Who knows what you'll get and what modifiers will be active 🤷

If we're confident moving forward with the width prop we can deprecate large now since it'll need to be deprecated for at least one major version cycle before it can be removed.

That sounds good to me, we can then make sure any modals in admin using large are switched over to the new prop. (AFAIK we only have 1, possibly a second). I guess partners would be notified that they need to switch over as well.

@alex-page I remember you had a comment about this possibly being an issue. Are we good to go with this approach?

We already have the large prop. I would like to see two PR's one to ship small at this moment then a breaking change to replace small and large.

That works for me. I can put up another PR for the breaking change next. Is this one good for adding the small prop?

thiagoloschi · 2021-05-06T20:53:26Z

src/components/Modal/components/Dialog/Dialog.tsx

  const classes = classNames(
    styles.Modal,
+    small && styles.sizeSmall,
    large && styles.sizeLarge,


In case both small and large are passed to the component, large should override small. I wonder where would be the best place to warn about that.

This makes sense to me as a failsafe, although if we end up using a width prop, I don't believe we'll need to have this logic.

^ Yup! It'll look something like

styles.Modal, styles[variationName('size', size)], limitHeight && styles.limitHeight,

alex-page · 2021-05-10T15:28:45Z

src/components/Modal/components/Dialog/Dialog.scss

 $vertical-spacing: 60px;
 $horizontal-spacing: spacing(extra-loose) * 2;
 $height-limit: 600px;
+$minimal-width: rem(380px);


Minimal is a little bit confusing. This isn't much better but might help?

Suggested change

$minimal-width: rem(380px);

$xsmall-width: rem(380px);

AndrewMusgrave

🚢 ✅

AndrewMusgrave · 2021-05-11T14:22:01Z

UNRELEASED.md

 - Add `noScroll` prop to `Modal` which prevents modal contents from scrolling ([#4153](https://github.com/Shopify/polaris-react/pull/4153))
 - Added new `color` prop to ProgressBar ([#3415](https://github.com/Shopify/polaris-react/pull/3415))
 - Added `requiredIndicator` prop to `Label`, `Labelled`, `Select` and `TextField` ([#4119](https://github.com/Shopify/polaris-react/pull/4119))
+- Add `small` prop to `Modal` so that width can be decreased to 380px ([#4146](https://github.com/Shopify/polaris-react/pull/4146))


Suggested change

- Add `small` prop to `Modal` so that width can be decreased to 380px ([#4146](https://github.com/Shopify/polaris-react/pull/4146))

- Add `small` prop to `Modal` so that width can be decreased to 380px ([#4177](https://github.com/Shopify/polaris-react/pull/4177))

ghost · 2021-05-11T15:17:40Z

🎉 Thanks for your contribution to Polaris React!

thiagoloschi self-assigned this May 6, 2021

thiagoloschi requested a review from alex-page May 6, 2021 18:19

thiagoloschi force-pushed the add-small-sized-modals branch from d92496a to cc1c480 Compare May 6, 2021 18:26

thiagoloschi requested review from AndrewMusgrave and lhoffbeck May 6, 2021 18:27

thiagoloschi force-pushed the add-small-sized-modals branch from cc1c480 to 8ffee3f Compare May 6, 2021 20:44

thiagoloschi commented May 6, 2021

View reviewed changes

alex-page reviewed May 10, 2021

View reviewed changes

alex-page approved these changes May 10, 2021

View reviewed changes

thiagoloschi force-pushed the add-small-sized-modals branch from 8ffee3f to 85381c2 Compare May 10, 2021 20:56

AndrewMusgrave approved these changes May 11, 2021

View reviewed changes

Add prop small to Modal

eb911e5

thiagoloschi force-pushed the add-small-sized-modals branch from 85381c2 to eb911e5 Compare May 11, 2021 14:36

thiagoloschi merged commit 498915d into main May 11, 2021

thiagoloschi deleted the add-small-sized-modals branch May 11, 2021 15:17

	- Add `small` prop to `Modal` so that width can be decreased to 380px ([#4146](https://github.com/Shopify/polaris-react/pull/4146))
	- Add `small` prop to `Modal` so that width can be decreased to 380px ([#4177](https://github.com/Shopify/polaris-react/pull/4177))

[Modal] Add prop small to decrease Modal's width #4177

[Modal] Add prop small to decrease Modal's width #4177

Uh oh!

Conversation

thiagoloschi commented May 6, 2021 • 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 🎩

🎩 checklist

Uh oh!

ghost commented May 6, 2021

Uh oh!

github-actions bot commented May 6, 2021 • 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: 3)

Files potentially affected (total: 0)

Files potentially affected (total: 5)

Files potentially affected (total: 4)

Files potentially affected (total: 0)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simon-callsen May 10, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

alex-page May 10, 2021 • 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!

AndrewMusgrave left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented May 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thiagoloschi commented May 6, 2021 •

edited

Loading

github-actions bot commented May 6, 2021 •

edited

Loading

simon-callsen May 10, 2021 •

edited

Loading

alex-page May 10, 2021 •

edited

Loading