Skip to content

chore(GlDisclosureDropdown): Simplify arg creation

Mark Florian requested to merge remove-generate-props into main

What does this MR do?

chore(GlDisclosureDropdown): Simplify arg creation

Storybook is already able to infer prop args via the component property of the default export of a story file. There's no need to explicitly read them ourselves.

This led to a problem for a lazily defined default value in !3244 (comment 1259093614).

It may be this pattern of explicitly reading default values arose before Storybook inferred component props.

Before After
Screenshot_2023-02-07_at_10-42-30_base___new-dropdowns___disclosure_-_Default___Storybook Screenshot_2023-02-07_at_10-41-47_base___new-dropdowns___disclosure_-_Default___Storybook

Discussion

Pros

  • Less code 🎉
  • Avoids a class of bugs (incorrect default value fetching).
  • It's clearer which props are not set by a given story, since they have a Set boolean/string/number/etc button instead of the default value in the control panel. (See for instance disabled in the screenshots above.)

Cons

  • Prop order is different, probably for each story, depending on the explicit args set for each story. The order is now determined by:

    1. Explicit arg source order.
    2. Remaining inferred args from component props in source order.

    This is technically the same as before. The difference now is that there are fewer explicit args set, so the source order of component props mattered less.

  • You can no longer at a glance see what the default value is for an optional prop in the controls panel. For instance, looking at the disabled control in the screenshots above, before you can see it's false by default, whereas now it's just a Set boolean button, which reveals a toggle. Though, the toggle does then show the correct default (false in this case).

Alternatives

  • We could create a story helper that implements the prop detection and default value fetching, avoiding things like !3277 (merged).

Does this MR meet the acceptance criteria?

Conformity

  • Code review guidelines.
  • GitLab UI's contributing guidlines.
  • If it changes a Pajamas-compliant component's look & feel, the MR has been reviewed by a UX designer.
  • If it changes GitLab UI's documentation guidelines, the MR has been reviewed by a Technical Writer.
  • If the MR changes a component's API, integration MR(s) have been opened in the following projects to ensure that the @gitlab/ui package can be upgraded quickly after the changes are released:
  • Added the ~"component:*" label(s) if applicable.

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • Security reports checked/validated by a reviewer from the AppSec team

Accessibility

If this MR adds or modifies a component, take a few moments to review the following:

  • All actions and functionality can be done with a keyboard.
  • Links, buttons, and controls have a visible focus state.
  • All content is presented in text or with a text equivalent. For example, alt text for SVG, or aria-label for icons that have meaning or perform actions.
  • Changes in a component’s state are announced by a screen reader. For example, changing aria-expanded="false" to aria-expanded="true" when an accordion is expanded.
  • Color combinations have sufficient contrast.
Edited by Mark Florian

Merge request reports