feat: add <InfoDialog> #367

emmenko · 2019-02-15T16:36:51Z

Based on #366

tdeekens · 2019-02-15T16:53:47Z

packages/application-components/src/components/dialogs/info-dialog/info-dialog.story.js

+import InfoDialog from './info-dialog';
+
+const DialogController = props => {
+  const [isOpen, toggle] = React.useState(false);


I wouldn't do this even though I like it. That means anybody building an app with our application-components needs to be on the latest React version. Not that it's unlikely but it's a bit of a harsh restriction balancing the benefit of it (mainly being funky) 😄

This is just the story for storybook, I don't think it's a problem to be honest.

True. Missed that. All good then. But it is worthwhile being on the lookout 😁

Our first hook component 🥺

tdeekens · 2019-02-15T16:54:24Z

packages/application-components/src/components/dialogs/info-dialog/info-dialog.story.js

+        justifyContent: 'center',
+        height: '100%',
+        width: '100%',
+      }}


Sorry, can we extract this? Either in a .mod.css or emotion. I don't feel like we have a rule for inline styles apart from: "it's not that much this time is it".

We plan to migrate to emotion, so this is just temporary.

I also don't mind using inline styles in stories.

tdeekens · 2019-02-15T16:55:11Z

@emmenko I would think it being helpful for other teams to also review this? So they know about the package, what's going on etc? Maybe just assign somebody from another team?

packages/application-components/src/components/dialogs/info-dialog/info-dialog.js

packages/application-components/src/components/dialogs/info-dialog/README.md

packages/application-components/src/components/dialogs/info-dialog/info-dialog.js

montezume · 2019-02-15T17:45:00Z

packages/application-components/src/components/dialogs/info-dialog/info-dialog.js

+              {props.title}
+            </Text.Subheadline>
+            <SecondaryIconButton
+              label="Close dialog"


shouldn't this be translated?

Maybe...maybe not. It's just for a11y.

I am fine with having it untranslated for now.

packages/application-components/src/components/dialogs/info-dialog/info-dialog.js

montezume · 2019-02-15T17:45:58Z

packages/application-components/src/components/dialogs/info-dialog/info-dialog.story.js

+        justifyContent: 'center',
+        height: '100%',
+        width: '100%',
+      }}


I also don't mind using inline styles in stories.

montezume · 2019-02-15T17:46:31Z

packages/application-components/src/index.js

 export { default as PageNotFound } from './components/page-not-found';
+
+// Dialogs
+export { default as InfoDialog } from './components/dialogs/info-dialog';


nice! not forgetting the export 💯

packages/application-components/src/components/dialogs/info-dialog/info-dialog.js

tdeekens

Looks good. Only one question.

tdeekens · 2019-02-18T12:30:32Z

packages/application-components/src/components/dialogs/info-dialog/info-dialog.js

+);
+InfoDialog.displayName = 'InfoDialog';
+InfoDialog.propTypes = {
+  title: PropTypes.string.isRequired,


Should we make this a node? This would require anybody to use formatMessage and apply the HoC over just rendering a <FormattedMessage />?

I had it as a node initially but @montezume asked to make it a string, because the value is also used for the HTML title attribute (which can only be a string).

Okay, sorry missed that. All good then.

qmateub

All looks good to me 👍 ! So cool that we have more a more components moved to application-kit so they can be reused 🔝

Left some questions, but for me this fine to 👌

qmateub · 2019-02-18T13:16:16Z

packages/application-components/src/components/dialogs/info-dialog/info-dialog.js

+import styles from './info-dialog.mod.css';
+
+// Like "Spacings.Inline", but with the `justify-content: space-between` style
+const Stretched = props => (


Any plans on having this in UI Kit at some point? I can verify that we will remove A LOT of custom styles in the MC 🤓 cc/ @montezume

@qmateub you can has an issue create 🐕

I guess we can also extend the Spacings.Inline component to support this mode

qmateub · 2019-02-18T13:20:59Z

packages/application-components/src/components/dialogs/info-dialog/info-dialog.js

+InfoDialog.propTypes = {
+  title: PropTypes.string.isRequired,
+  children: PropTypes.node.isRequired,
+  horizontalConstraint: PropTypes.oneOf(['m', 'l']),


So, we only support m and l sizes here? Is it a design requirement? I'm saying because I think the original component does not have any of that right? 🤔

Yes, it's a design requirement

qmateub · 2019-02-18T13:21:57Z

packages/application-components/src/components/dialogs/info-dialog/info-dialog.spec.js

+
+    fireEvent.click(getByLabelText(/Close dialog/));
+    await wait(() => {
+      expect(queryByText(/Lorem ipsus/)).not.toBeInTheDocument();


Oh! I didn't know that you can await and do an expect 😅 . I will keep this in mind for future test writes 👍

Yeah, this comes quite in handy

qmateub · 2019-02-18T13:22:39Z

packages/application-components/src/components/dialogs/info-dialog/info-dialog.story.js

+import InfoDialog from './info-dialog';
+
+const DialogController = props => {
+  const [isOpen, toggle] = React.useState(false);


Our first hook component 🥺

emmenko · 2019-02-19T12:41:01Z

FYI: I'll probably wait for a couple of fixes in ui-kit before merging this

…n clicking on the overlay

… parts

…of content

emmenko added 🙏 Status: Dev Review Waiting for technical reviews 🚀 Type: New Feature Something new labels Feb 15, 2019

emmenko force-pushed the nm-info-dialog branch from f35d518 to 25c48f6 Compare February 15, 2019 16:48

emmenko requested a review from montezume February 15, 2019 16:49

tdeekens reviewed Feb 15, 2019

View reviewed changes

packages/application-components/src/components/dialogs/info-dialog/info-dialog.js Outdated Show resolved Hide resolved

emmenko requested a review from aminbenselim February 15, 2019 17:05

montezume reviewed Feb 15, 2019

View reviewed changes

packages/application-components/src/components/dialogs/info-dialog/README.md Outdated Show resolved Hide resolved

montezume reviewed Feb 15, 2019

View reviewed changes

packages/application-components/src/components/dialogs/info-dialog/README.md Outdated Show resolved Hide resolved

montezume reviewed Feb 15, 2019

View reviewed changes

packages/application-components/src/components/dialogs/info-dialog/info-dialog.js Outdated Show resolved Hide resolved

tdeekens reviewed Feb 18, 2019

View reviewed changes

montezume approved these changes Feb 18, 2019

View reviewed changes

qmateub approved these changes Feb 18, 2019

View reviewed changes

emmenko force-pushed the nm-storybook branch from 25b33ec to 00049d3 Compare February 18, 2019 13:28

emmenko force-pushed the nm-info-dialog branch from 011e07d to 9aedef9 Compare February 18, 2019 14:03

emmenko changed the base branch from nm-storybook to master February 18, 2019 14:06

qmateub mentioned this pull request Feb 18, 2019

Spacings component support option for space-between layout configuration commercetools/ui-kit#552

Closed

This was referenced Feb 18, 2019

feat: add <ConfirmationDialog> #377

Merged

Fix flex padding/margin overflow in Card component commercetools/ui-kit#554

Closed

emmenko added 8 commits February 20, 2019 15:08

chore: setup storybook for developing application components

37ca816

chore: move storybook into application-components package

0f967df

feat(app-components): add InfoDialog component

b242926

refactor(app-components): apply changes to InfoDialog

9da2e79

test(components/info-dialog): add test

92aa903

fix(login): props to InfoDialog

3bc7d99

refactor(components/info-dialog): remove unnecessary close timeout

fb94269

fix(components/info-dialog): pointer events for closing the modal whe…

d713530

…n clicking on the overlay

emmenko added 4 commits February 20, 2019 15:08

refactor(components/info-dialog): to split up internals into reusable…

f770bbf

… parts

refactor(components/info-dialog): support constraint scale

079eecc

chore: update lockfile

5ce6dc7

refactor(components/info-dialog): adjust max height/width dimensions …

f32929e

…of content

emmenko force-pushed the nm-info-dialog branch from 0eb792d to f32929e Compare February 20, 2019 14:27

refactor(components/info-dialog): to use new justifyContent prop

fd34e44

emmenko merged commit ba359a3 into master Feb 20, 2019

emmenko deleted the nm-info-dialog branch February 20, 2019 14:53

emmenko mentioned this pull request Feb 26, 2019

Adding UI components for building applications #323

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add <InfoDialog> #367

feat: add <InfoDialog> #367

emmenko commented Feb 15, 2019 •

edited

Loading

tdeekens Feb 15, 2019

emmenko Feb 15, 2019

tdeekens Feb 16, 2019

qmateub Feb 18, 2019

tdeekens Feb 15, 2019

emmenko Feb 15, 2019

montezume Feb 15, 2019

tdeekens commented Feb 15, 2019

montezume Feb 15, 2019

emmenko Feb 15, 2019

tdeekens Feb 18, 2019

montezume Feb 15, 2019

montezume Feb 15, 2019

tdeekens left a comment

tdeekens Feb 18, 2019

emmenko Feb 18, 2019

tdeekens Feb 18, 2019

qmateub left a comment

qmateub Feb 18, 2019

montezume Feb 18, 2019

emmenko Feb 18, 2019

qmateub Feb 18, 2019

emmenko Feb 18, 2019

qmateub Feb 18, 2019

emmenko Feb 18, 2019

qmateub Feb 18, 2019

emmenko commented Feb 19, 2019

feat: add <InfoDialog> #367

feat: add <InfoDialog> #367

Conversation

emmenko commented Feb 15, 2019 • edited Loading

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

tdeekens commented Feb 15, 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

tdeekens left a 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

qmateub left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmenko commented Feb 19, 2019

emmenko commented Feb 15, 2019 •

edited

Loading