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

refactor(storybook): build storybook simple story args interfaces using component class #9457

Conversation

aPreciado88
Copy link
Contributor

@aPreciado88 aPreciado88 commented May 29, 2024

Related Issue: #9437

Summary

Replaces Storybook's manually built simple story args interfaces with props pulled from the component's class.

…eciado88/9084-set-up-controls-for-simple-storybook-stories
…eciado88/9084-set-up-controls-for-simple-storybook-stories
…eciado88/9084-set-up-controls-for-simple-storybook-stories
@aPreciado88 aPreciado88 requested a review from a team as a code owner May 29, 2024 20:32
@github-actions github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label May 29, 2024
@aPreciado88 aPreciado88 added pr ready for visual snapshots Adding this label will run visual snapshot testing. skip visual snapshots Pull requests that do not need visual regression testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels May 29, 2024
…eciado88/9437-build-storybook-interfaces-using-component-class
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Awesome!

Can you 1. update the story args interfaces to follow the same picking pattern and 2. ensure all component props used in stories are picked from the class vs declaring them separately?

Once the above is addressed, this is good to merge!

loading: boolean;
disabled: boolean;
headingLevel: number;
interface BlockStoryArgs {
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be simplified a bit by using Pick:

interface BlockStoryArgs extends 
  Pick<Block, "heading" | "description" | "open" | "collapsible" | "loading" | "disabled" | "headingLevel">, 
  Pick<BlockSection, "open" | "toggleDisplay"> {
  text: string;
}

Applies to similar args types.

Copy link
Contributor Author

@aPreciado88 aPreciado88 Jun 4, 2024

Choose a reason for hiding this comment

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

Both Block and BlockSection have an open prop, I had to declare them as blockOpen: Block["open"]; and sectionOpen: BlockSection["open"]; to be able to use them in storybook's ui. If I use the Pick format the open props conflict and storybook's ui doesn't work.

I could pull Block's open prop with Pick and separately declare BlockSection's open prop as sectionOpen to avoid conflicts. Please let me know If this is okay, I can follow that same pattern with all other story interfaces.

  extends Pick<Block, "heading" | "description" | "open" | "collapsible" | "loading" | "disabled" | "headingLevel">,
    Pick<BlockSection, "toggleDisplay"> {
  sectionOpen: boolean;
  text: string;
}

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying! Your suggestion looks good.

You could also use BlockSection's type to keep the renamed prop in sync and also make it clear that it's coming from section.

 extends Pick<Block, "heading" | "description" | "open" | "collapsible" | "loading" | "disabled" | "headingLevel">,
    Pick<BlockSection, "toggleDisplay"> {
  sectionOpen: BlockSection["open"];
  text: string;
}

controlOverlay: Carousel["controlOverlay"];
disabled: Carousel["disabled"];
autoplayDuration: Carousel["autoplayDuration"];
autoplay: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Carousel should have an autoplay prop.

This might apply to other classes/props too (e.g., Tooltip).

Copy link
Contributor

@alisonailea alisonailea left a comment

Choose a reason for hiding this comment

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

Builds locally and looks clean! Nice.

@aPreciado88 aPreciado88 added this to the 2024-06-25 - Jun Release milestone Jun 4, 2024
…eciado88/9437-build-storybook-interfaces-using-component-class
…eciado88/9437-build-storybook-interfaces-using-component-class
…eciado88/9437-build-storybook-interfaces-using-component-class
@aPreciado88 aPreciado88 merged commit 99eac2e into main Jun 7, 2024
9 checks passed
@aPreciado88 aPreciado88 deleted the aPreciado88/9437-build-storybook-interfaces-using-component-class branch June 7, 2024 19:14
benelan added a commit that referenced this pull request Jun 10, 2024
…orkflow

* origin/main: (26 commits)
  revert: refactor: add simpler `componentFocusable` util (deprecates `LoadableComponent`) (#9515)
  chore(linting): enable selector-pseudo-element-colon-notation rule (#9518)
  refactor(storybook): refactor tooltip simple story interface (#9538)
  refactor(dom): consolidate transition/animation waiting utils (#9341)
  refactor(storybook): build storybook simple story args interfaces using component class (#9457)
  chore: release next
  fix(block): add accessible label for slotted controls (#9502)
  feat(action-bar, action-pad): add expandLabel and collapseLabel to messages (#9497)
  chore: release next
  feat(action-menu, combobox, dropdown, input-date-picker, popover): allow logical placements for flipPlacements property. #8825 (#9490)
  fix(popover): correct border radius on close button (#9485)
  fix(list-item): hide nested list items by default (#9474)
  refactor: move component types into component specific interfaces files (#9527)
  chore: release next
  fix(alert): pause auto-close alert when link focused (#9503)
  refactor(storybook): consolidate storybook and component types (#9500)
  refactor(calcite-block-section,calcite-card): consolidate interfaces imports (#9517)
  chore: release next
  fix(block-section): restore block toggling when clicking on the header switch (#9472)
  chore: release next
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Issues tied to code that needs to be significantly reworked. skip visual snapshots Pull requests that do not need visual regression testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants