fix: element component + subcomponent type conventions #1273

jzempel · 2022-01-21T15:14:48Z

Description

A sweep over all exported Garden element components to bring the code inline with the internal "Element component + subcomponent type conventions" coding RFC. Standard coding conventions are outlined in the updated API docs, which provide a good starting point for framing the PR review.

Detail

A website follow-on from this PR is expected to auto (rather than manually) generate all subcomponent prop sheet documentation, which should represent a significant uptick in efficiency and consistency.

Checklist

👌 design updates are Garden Designer approved (add the
designer as a reviewer)
🌐 demo is up-to-date (yarn start)
⬅️ renders as expected with reversed (RTL) direction
🤘 renders as expected with Bedrock CSS (?bedrock)
♿ ~~analyzed via axe and evaluated using VoiceOver~~
💂‍♂️ ~~includes new unit tests~~
📝 ~~tested in Chrome, Firefox, Safari, Edge, and IE11~~

jzempel · 2022-01-21T15:19:53Z

packages/accordions/src/elements/timeline/components/Item.tsx

+/**
+ * @deprecated use ITimelineItemProps instead
+ */
+export type IItem = ITimelineItemProps;


IElementSubElement naming is used rather than ISubElement for consistent subcomponent type exports that avoid naming confusion or conflicts with other subcomponent types. i.e. Timeline.Item vs Stepper.Item just as a contrived example.

jzempel · 2022-01-21T15:21:08Z

packages/avatars/src/index.ts

@@ -5,5 +5,5 @@
 * found at http://www.apache.org/licenses/LICENSE-2.0.
 */

-export { default as Avatar } from './elements/Avatar';


no more mix of default vs named exports

I think MonthSelector is the only missing default export for components.

As MonthSelector was neither an exported component or subcomponent, I didn't have it in view for this PR. But I'll go ahead and update the outlier.

jzempel · 2022-01-21T15:25:26Z

packages/chrome/src/elements/header/HeaderItemIcon.tsx

+export const HeaderItemIcon = ({
+  children,
+  ...props
+}: PropsWithChildren<HTMLAttributes<HTMLElement>>) => {
+  const element = Children.only(children) as ReactHTMLElement<HTMLElement>;

-  return <StyledHeaderItemIcon as={Element as any} {...props} />;
+  if (isValidElement(element)) {
+    const Icon = ({
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      theme,
+      ...iconProps
+    }: ThemeProps<DefaultTheme> & HTMLAttributes<HTMLElement>) =>
+      cloneElement<HTMLAttributes<HTMLElement>, HTMLElement>(element, { ...props, ...iconProps });
+
+    return <StyledHeaderItemIcon as={Icon} {...props} />;
+  }


Direct clone of Icon type elements is a future inconsistency that should be addressed throughout the codebase. This change is simply buttoning-up the types for props.

Could you explain more on this matter?

I could 😉 What are you looking for exactly? Perhaps we should talk offline. I've captured some of the inconsistencies in our version planning doc.

Nothing in particular, was just wondering, however if we have this recorded, we should be okay.

coveralls · 2022-01-21T15:51:37Z

Coverage decreased (-0.003%) to 95.768% when pulling db114b2 on jzempel/element-types into c711002 on main.

Francois-Esquire · 2022-01-24T15:42:48Z

docs/api.md

+3. The `displayName` definition should be set to the exported component name and
+   satisfies linter checks for runtime naming


I am a little confused here.. "should be set to the exported component name", would that be Element or ElementComponent?

The only exported component is Element – so that should be the displayName. Do I need to take another crack at the verbiage here?

Hm, I may have taken it too literally with how it is worded? The linting work correctly so it shouldn't be an issue... I just understood it as (example):

/** * @extends HTMLAttributes<HTMLDivElement> */ export const Accordion = AccordionComponent as typeof AccordionComponent & { ... }; Accordion.displayName = 'Accordion';

Instead of what we are actually doing:

AccordionComponent.displayName = 'Accordion'; /** * @extends HTMLAttributes<HTMLDivElement> */ export const Accordion = AccordionComponent as typeof AccordionComponent & { ... };

Since Accordion is what is being exported. Possibly making a distinction between element's and styled-components` would help.. not too sure frankly.

Your (example) can't happen – it breaks both displayName linting and JSDoc parsing.

We actually need the "actually doing" version. And the point is to highlight the necessary AccordionComponent.displayName = 'Accordion'; mismatch – which satisfies both linting and provides the necessary connective tissue for JSDoc parsing via garden cmd-docgen.

Francois-Esquire · 2022-01-24T15:52:17Z

docs/documentation.md

+  - Only add prop JSDoc to Typescript prop interfaces
+    - Refrain from documenting React `defaultProps`
+    - Refrain from documenting styled components props
+  - Use `@ignore` to prevent a prop from being added to generated documentation


When would @ignore usage be appropriate? I did notice ButtonGroups isSelected prop as one usage within react-components, would this hide functionality or is it intended for the internal API?

This rare notation hides props that should only be used internally and not intended for external use. Should I state that here?

I think it would help to state that, maybe using the internal props as an example would frame its purpose better.

docs/api.md

packages/chrome/src/styled/header/StyledBaseHeaderItem.ts

packages/forms/src/index.ts

packages/modals/src/elements/DrawerModal/Header.tsx

packages/.template/src/elements/{{capitalize component}}Text.tsx

mtomcal

This is some amazing code collapse here. 👏

mtomcal · 2022-01-24T18:59:55Z

packages/buttons/src/elements/IconButton.tsx

@@ -64,5 +62,3 @@ IconButton.defaultProps = {
  isBasic: true,
  size: 'medium'
 };
-
-export default IconButton;


We should be prepared with a boilerplate message (not in code) if any consumers directly import default from these components that they will be broken. They should not be importing files directly in the first place but we might see chatter about this regardless for this and any migrations of subcomponents to their new files.

mtomcal · 2022-01-24T19:04:27Z

packages/chrome/src/elements/nav/NavItemIcon.tsx

+      cloneElement<HTMLAttributes<HTMLElement>, HTMLElement>(element, { ...props, ...iconProps });
+
+    return <StyledNavItemIcon as={Icon} {...props} />;
+  }


What is the objective of this expanded logic?

Seems this is a repeat of #1273 (comment) but different file. Would like to hear the offline breakdown of what this is doing.

Exact same as #1273 (comment)

hzhu

These are much needed updates that keep the project structured well. Thanks!

docs/documentation.md

Francois-Esquire · 2022-01-26T15:17:13Z

@jzempel this PR brings much needed refinement to the project/packages, thank you 🙌

jzempel added 30 commits January 9, 2022 10:11

Update Accordion element component + subcomponent type conventions

768d5db

Update Stepper element component + subcomponent type conventions

1e6bd41

Update Timeline element component + subcomponent type conventions

9a02d1a

Align subcomponent prop descriptions with website

e417b2e

Fix Timeline circular reference

d2c4080

Update Avatar element component + subcomponent type conventions

fdd66de

Hack subcomponent exports for Storybook prop display

6a64999

Fix type export

2880dfc

Update Button element component + subcomponent type conventions

490d5dd

Update Colorpicker element component + subcomponent type conventions

0ddd5f5

Fix circular dependency

0f76ecf

Update Datepicker element component + subcomponent type conventions

948b4ce

Update Dropdown element component + subcomponent type conventions

afe876c

Fix dropdown Field testing

2d38efa

Update forms element component + subcomponent type conventions

20bb8fe

Update loaders element component + subcomponent type conventions

c9f7464

Update modals element component + subcomponent type conventions

80f00df

Update notification element component + subcomponent type conventions

044bbe1

Update notification element component + subcomponent type conventions

ed6afc9

Update notification element component + subcomponent type conventions

201c72b

Update pagination element component + subcomponent type conventions

336bb9e

Update pagination element component + subcomponent type conventions

c39055c

Update table element component + subcomponent type conventions

12bdff7

Update tabs element component + subcomponent type conventions

1ca4bf1

Update tags element component + subcomponent type conventions

307f689

Update theming element component + subcomponent type conventions

446a9e9

Update tooltip element component + subcomponent type conventions

fb084a5

Update typography element component + subcomponent type conventions

96d896b

Remove forwardRef for cloned icons

47623c3

Update template subcomponent structure

b946422

jzempel requested a review from a team as a code owner January 21, 2022 15:14

jzempel commented Jan 21, 2022

View reviewed changes

zendesk-garden deployed to staging January 21, 2022 15:53 View deployment

jzempel mentioned this pull request Jan 24, 2022

feat(sheet): add documentation zendeskgarden/website#337

Merged

5 tasks

Francois-Esquire reviewed Jan 24, 2022

View reviewed changes

packages/.template/src/elements/{{capitalize component}}Text.tsx Show resolved Hide resolved

mtomcal reviewed Jan 24, 2022

View reviewed changes

Address comments from @Francois-Esquire

0296143

hzhu approved these changes Jan 24, 2022

View reviewed changes

docs/documentation.md Outdated Show resolved Hide resolved

zendesk-garden deployed to staging January 24, 2022 21:20 View deployment

jzempel added 2 commits January 24, 2022 16:24

Address @hzhu copy nit

f06df5d

Merge branch 'main' into jzempel/element-types

102c90d

zendesk-garden deployed to staging January 24, 2022 22:13 View deployment

Address @Francois-Esquire documentation comment

4b96955

zendesk-garden deployed to staging January 25, 2022 15:23 View deployment

Remove element extension props

deff0e5

zendesk-garden deployed to staging January 25, 2022 21:16 View deployment

Francois-Esquire approved these changes Jan 26, 2022

View reviewed changes

Merge branch 'main' into jzempel/element-types

4107c0a

zendesk-garden deployed to staging January 26, 2022 16:33 View deployment

Merge branch 'main' into jzempel/element-types

db114b2

zendesk-garden deployed to staging January 26, 2022 19:32 View deployment

mtomcal approved these changes Jan 26, 2022

View reviewed changes

jzempel merged commit cb3360c into main Jan 26, 2022

jzempel deleted the jzempel/element-types branch January 26, 2022 20:07

jzempel mentioned this pull request Feb 2, 2022

fix(datepickers): add missing DatepickerRange subcomponent display names #1289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: element component + subcomponent type conventions #1273

fix: element component + subcomponent type conventions #1273

jzempel commented Jan 21, 2022

jzempel Jan 21, 2022

jzempel Jan 21, 2022

Francois-Esquire Jan 24, 2022

jzempel Jan 24, 2022

jzempel Jan 21, 2022

Francois-Esquire Jan 24, 2022

jzempel Jan 24, 2022

Francois-Esquire Jan 25, 2022

coveralls commented Jan 21, 2022 •

edited

Francois-Esquire Jan 24, 2022

jzempel Jan 24, 2022 •

edited

Francois-Esquire Jan 24, 2022

jzempel Jan 24, 2022

Francois-Esquire Jan 24, 2022

jzempel Jan 24, 2022

Francois-Esquire Jan 24, 2022 •

edited

mtomcal left a comment

mtomcal Jan 24, 2022 •

edited

mtomcal Jan 24, 2022

mtomcal Jan 24, 2022

jzempel Jan 24, 2022

hzhu left a comment

Francois-Esquire commented Jan 26, 2022

		3. The `displayName` definition should be set to the exported component name and
		satisfies linter checks for runtime naming

fix: element component + subcomponent type conventions #1273

fix: element component + subcomponent type conventions #1273

Conversation

jzempel commented Jan 21, 2022

Description

Detail

Checklist

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

coveralls commented Jan 21, 2022 • edited

Choose a reason for hiding this comment

jzempel Jan 24, 2022 • edited

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

Francois-Esquire Jan 24, 2022 • edited

Choose a reason for hiding this comment

mtomcal left a comment

Choose a reason for hiding this comment

mtomcal Jan 24, 2022 • edited

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

hzhu left a comment

Choose a reason for hiding this comment

Francois-Esquire commented Jan 26, 2022

coveralls commented Jan 21, 2022 •

edited

jzempel Jan 24, 2022 •

edited

Francois-Esquire Jan 24, 2022 •

edited

mtomcal Jan 24, 2022 •

edited