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

Add storybook story for Disabled component #20514

Merged
merged 4 commits into from
Mar 25, 2020

Conversation

brentswisher
Copy link
Contributor

Description

Adds a storybook story for the Disabled component as part of #17973

I had to make a decision on this story regarding what/how much to put in the component to show as disabled. I included all the "basic" form controls as well as a few of the more complex ones. I don't have a strong feeling on what should be shown, so if anyone has feedback on more/less child components just let me know.

Concerns

I also ran into a few things with the storyshot integration I want to call out:

  • It seems including another component in a new story increments an identifier used in the other stories:
    Comparing_master___add_disabled-component-story_·_WordPress_gutenberg_1
    I am pretty sure this is intentional but didn't see it documented everywhere so I just wanted to double-check.
  • The Disabled component uses a MutationObserver which jest doesn't support. Using the unit test for the Disabled component as a template I added a stub of window.MutationObserver in beforeAll() and removed it in afterAll(). This appears to work but doesn't seem ideal as none of the other storybook tests need it. If there is a better way to do this feel free to point me in that direction.

How has this been tested?

  • run npm run storybook:dev
  • See the new Disabled story under components

Screenshots

Components___Disabled_-_Default_⋅_Storybook

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented Feb 27, 2020

Size Change: 0 B

Total Size: 856 kB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 998 B 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 100 kB 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 7.23 kB 0 B
build/block-library/editor.css 7.24 kB 0 B
build/block-library/index.js 111 kB 0 B
build/block-library/style-rtl.css 7.42 kB 0 B
build/block-library/style.css 7.43 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.5 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.2 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.2 kB 0 B
build/edit-post/style-rtl.css 8.47 kB 0 B
build/edit-post/style.css 8.46 kB 0 B
build/edit-site/index.js 5.07 kB 0 B
build/edit-site/style-rtl.css 2.53 kB 0 B
build/edit-site/style.css 2.53 kB 0 B
build/edit-widgets/index.js 4.43 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/index.js 43.8 kB 0 B
build/editor/style-rtl.css 3.97 kB 0 B
build/editor/style.css 3.96 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 6.96 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.69 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/notices/index.js 1.58 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@gziolo
Copy link
Member

gziolo commented Mar 5, 2020

As discussed on Slack yesterday, we will need to upgrade Jest to use the latest version of JSDom which supports MutationObserver. I guess you can add a code comment and create a follow-up issue if we don't want to wait with this PR.

@brentswisher
Copy link
Contributor Author

I updated jest, jsdom and storybook to the most recent versions locally and am still getting this error without the setup/teardown methods:

`● Storyshots › Components/Disabled › Default

TypeError: window.MutationObserver is not a constructor

  47 | 		this.disable();
  48 |
> 49 | 		this.observer = new window.MutationObserver( this.debouncedDisable );
     | 		                ^
  50 | 		this.observer.observe( this.node, {
  51 | 			childList: true,
  52 | 			attributes: true,`

The js configuration for Gutenberg is a bit more complex than I am used to working with, so it's possible I am not updating what I think I am or in the correct place.

@gziolo
Copy link
Member

gziolo commented Mar 10, 2020

I opened #20766 which tries to upgrade Jest and JSDom to the more recent versions.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It all looks great. I left two comments to respond. We should document the issue with MutationObserver and file a follow-up issue to unblock this PR.

storybook/test/index.js Show resolved Hide resolved

export const _default = () => {
return (
<Disabled>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need all those components to include in here? Can we use the most representative 3? I don't have answers, I'm thinking whether it's enough to cover those that use input, select and textarea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I questioned that too, and nearly added just a single input. I would be fine with making it just the top three, maybe @ItsJonQ might have an opinion?

Copy link

Choose a reason for hiding this comment

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

Hai! 👋
I think max 3 would be okay. Let's go with that 👍

Copy link
Contributor Author

@brentswisher brentswisher Mar 18, 2020

Choose a reason for hiding this comment

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

Updated 🙂

@gziolo gziolo requested a review from ItsJonQ March 13, 2020 07:02
Copy link

@ItsJonQ ItsJonQ left a comment

Choose a reason for hiding this comment

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

🚀 Tested and looks good to me! Thank you @brentswisher !

@ItsJonQ
Copy link

ItsJonQ commented Mar 18, 2020

Will merge once Travis is 💚 !

@brentswisher brentswisher merged commit 20908e7 into master Mar 25, 2020
@brentswisher brentswisher deleted the add/disabled-component-story branch March 25, 2020 13:01
torounit added a commit that referenced this pull request Aug 18, 2022
ciampo added a commit that referenced this pull request Aug 23, 2022
* refactor storybook

* convert to typescript

* fix ComponentProps

* add changelog.

* fix imports order

* use WordPressComponentProps.

* add example

* Update packages/components/src/disabled/types.ts

Co-authored-by: Marco Ciampini <[email protected]>

* Update packages/components/src/disabled/index.tsx

Co-authored-by: Marco Ciampini <[email protected]>

* Update packages/components/src/disabled/index.tsx

Co-authored-by: Petter Walbø Johnsgård <[email protected]>

* Update packages/components/src/disabled/index.tsx

Co-authored-by: Petter Walbø Johnsgård <[email protected]>

* Remove unused imports.

* Rewrite to Testing-library `will disable all fields`

* Rewrite to Testing-library `should cleanly un-disable via reconciliation`

* use rerender

* refactor test.

* replace react-dom/test-utils to testing-library.

* remove unnecessary MutationObserver stab.
@see #20766 #20514

* Convert to typescript.

* add story for contentEditable

* add control settings.

* fix changelog

* Update packages/components/CHANGELOG.md

Co-authored-by: Marco Ciampini <[email protected]>

* Omit ref.

* avoid querying

* rename div.

* test before rerender

* Simplify

* Update packages/components/src/disabled/test/index.tsx

Co-authored-by: Marco Ciampini <[email protected]>

* test for sneaky DOM manipulation

* Fix `isDisabled` so that it keeps its value even if it is changed.

* add default args

* Revert "Fix `isDisabled` so that it keeps its value even if it is changed."

This reverts commit 28820f0.

* Update packages/components/src/disabled/test/index.tsx

* Update packages/components/CHANGELOG.md

Co-authored-by: Marco Ciampini <[email protected]>
Co-authored-by: Petter Walbø Johnsgård <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components Storybook Storybook and its stories for components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants