feat(component): restrict actions and header from modals #209

jorgemoya · 2019-10-07T18:17:24Z

BREAKING CHANGES

What

Headers and Actions are now a prop of modal component. Visually no change.

Added header & actions props.

actions accepts an array of Objects with Button props and a text prop.

Example:

[
  { text: 'Cancel', onClick: () => setIsOpen(false) },
  { text: 'Apply', onClick: () => setIsOpen(false) },
]

Updated tests and docs.

Screenshot

deini · 2019-10-07T18:27:54Z

packages/big-design/src/components/Modal/Modal.tsx

  variant: 'modal' | 'dialog';
+  withActionsBorder?: boolean;


Do we have multiple designs? I think this should be consistent in all modals and not configurable.

@deini It seems like you added it in #133 to support @animesh1987 use case. Is this something we need to touch back with design?

Removed borders per offline discussion.

packages/big-design/src/components/Modal/Modal.tsx

deini · 2019-10-07T18:29:39Z

Can you also add the BREAKING CHANGE section to your commit body with examples of how to migrate? 🙏

jorgemoya · 2019-10-07T18:50:08Z

Can you also add the BREAKING CHANGE section to your commit body with examples of how to migrate? 🙏

Is this done when you squash and merge the commit?

packages/big-design/src/components/Modal/Modal.tsx

packages/big-design/src/components/Modal/spec.tsx

chanceaclark · 2019-10-07T18:47:15Z

packages/docs/PropTables/ModalPropTable.tsx

-    <PropTable.Prop name="onClose" types="() => void" required>
-      Function that will be called on close events.
+    <PropTable.Prop name="actions" types="React.ReactNode">
+      Sets the actions of the modal.


Maybe we should reword this to what should be passed into the actions (i.e. Button's).

packages/docs/PropTables/ModalPropTable.tsx

chanceaclark · 2019-10-07T18:52:48Z

Can you also add the BREAKING CHANGE section to your commit body with examples of how to migrate? 🙏

Is this done when you squash and merge the commit?

Yeah, you can write it in full markdown.

Co-Authored-By: Chancellor Clark <chancellorclark@gmail.com>

jorgemoya · 2019-10-07T20:58:26Z

packages/docs/PropTables/ModalPropTable.tsx

+    <PropTable.Prop name="actions" types="object[]">
+      Accepts an array of objects with <NextLink href="/button">Button</NextLink> props and a <Code>text</Code> label.
    </PropTable.Prop>


What do you guys think about this prop description?

deini · 2019-10-07T21:15:01Z

packages/big-design/src/components/Modal/Modal.tsx

-export interface ModalHeaderProps {
-  withBorder?: boolean;
+interface Action extends Omit<ButtonProps, 'children'> {
+  text?: string;


Should this be required?

I was thinking we could have only icon buttons

Good point, can't think of it being a good use case but let's leave it for now. Good catch.

deini · 2019-10-07T21:15:20Z

packages/big-design/src/components/Modal/Modal.tsx

-
-export interface ModalHeaderProps {
-  withBorder?: boolean;
+interface Action extends Omit<ButtonProps, 'children'> {


What do you think about renaming this ModalAction? We also need to export it.

Why export it?

For example, if you don't want to inline the actions prop you could do.

const actions: ModalAction[] = [ { text: 'Cancel', onClick: () => {} } ]; // And use it like <Modal actions={actions} />

packages/big-design/src/components/Modal/Modal.tsx

icatalina

The PR looks awesome, I'll defer to @deini for the final approval.

icatalina · 2019-10-07T23:20:45Z

packages/docs/pages/Modal/ModalPage.tsx

-                </Button>
-                <Button onClick={() => setIsOpen(false)}>Apply</Button>
-              </Modal.Actions>
+            <Modal


This looks great! 👍

chanceaclark · 2019-10-07T23:10:39Z

packages/big-design/src/components/Modal/spec.tsx

+  const { getAllByRole } = render(<Modal isOpen={true} actions={[{ text: 'Cancel' }, { text: 'Apply' }]} />);
+
+  expect(getAllByRole('button').length).toBe(3);
+});


🍹Could we add some additional tests? One to make sure the button callbacks fire, another to ensure props like variants and actionType have correct stylings, and another to ensure we can't pass a component into the header?

chanceaclark · 2019-10-07T23:15:42Z

packages/docs/PropTables/ModalPropTable.tsx

-    <PropTable.Prop name="onClose" types="() => void" required>
-      Function that will be called on close events.
+    <PropTable.Prop name="actions" types="object[]">
+      Accepts an array of objects with <NextLink href="/button">Button</NextLink> props and a <Code>text</Code> label.


Accepts an array of objects with Button props and a text label.

Maybe like this:
Accepts an array of objects with Button props with an additional text prop. See example for usage.
🤷

feat(component): restrict actions and header from modals

d7ef9b2

jorgemoya requested a review from a team October 7, 2019 18:17

deini reviewed Oct 7, 2019

View reviewed changes

fix: type check header prop

62c7741

chanceaclark reviewed Oct 7, 2019

View reviewed changes

jorgemoya and others added 4 commits October 7, 2019 13:54

Update packages/docs/PropTables/ModalPropTable.tsx

14174b4

Co-Authored-By: Chancellor Clark <chancellorclark@gmail.com>

feat: remove borders

c2a1d15

feat: actions are now an array of objects with button props

6d872f6

fix: tslint errors

5d8f174

jorgemoya commented Oct 7, 2019

View reviewed changes

deini reviewed Oct 7, 2019

View reviewed changes

icatalina reviewed Oct 7, 2019

View reviewed changes

chanceaclark reviewed Oct 8, 2019

View reviewed changes

fix: pr feedback

56ef3a7

jorgemoya force-pushed the restrict-modal branch from 9abee2f to 56ef3a7 Compare October 8, 2019 15:03

fix: add additional tests

540c594

deini approved these changes Oct 8, 2019

View reviewed changes

fix: missing test

b8bd582

chanceaclark approved these changes Oct 8, 2019

View reviewed changes

jorgemoya merged commit bc85d25 into bigcommerce:master Oct 8, 2019

jorgemoya deleted the restrict-modal branch October 8, 2019 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(component): restrict actions and header from modals #209

feat(component): restrict actions and header from modals #209

jorgemoya commented Oct 7, 2019 •

edited

Loading

deini Oct 7, 2019

chanceaclark Oct 7, 2019

jorgemoya Oct 7, 2019 •

edited

Loading

deini commented Oct 7, 2019

jorgemoya commented Oct 7, 2019

chanceaclark Oct 7, 2019

chanceaclark commented Oct 7, 2019

jorgemoya Oct 7, 2019

deini Oct 7, 2019

jorgemoya Oct 7, 2019

deini Oct 7, 2019

deini Oct 7, 2019

jorgemoya Oct 7, 2019

deini Oct 7, 2019

icatalina left a comment

icatalina Oct 7, 2019

chanceaclark Oct 7, 2019 •

edited

Loading

chanceaclark Oct 7, 2019

feat(component): restrict actions and header from modals #209

feat(component): restrict actions and header from modals #209

Conversation

jorgemoya commented Oct 7, 2019 • edited Loading

BREAKING CHANGES

What

Screenshot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgemoya Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

deini commented Oct 7, 2019

jorgemoya commented Oct 7, 2019

Choose a reason for hiding this comment

chanceaclark commented Oct 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icatalina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanceaclark Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgemoya commented Oct 7, 2019 •

edited

Loading

jorgemoya Oct 7, 2019 •

edited

Loading

chanceaclark Oct 7, 2019 •

edited

Loading