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

fix: element component + subcomponent type conventions #1273

Merged
merged 41 commits into from
Jan 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
768d5db
Update Accordion element component + subcomponent type conventions
jzempel Jan 9, 2022
1e6bd41
Update Stepper element component + subcomponent type conventions
jzempel Jan 9, 2022
9a02d1a
Update Timeline element component + subcomponent type conventions
jzempel Jan 9, 2022
e417b2e
Align subcomponent prop descriptions with website
jzempel Jan 9, 2022
d2c4080
Fix Timeline circular reference
jzempel Jan 9, 2022
fdd66de
Update Avatar element component + subcomponent type conventions
jzempel Jan 9, 2022
6a64999
Hack subcomponent exports for Storybook prop display
jzempel Jan 10, 2022
2880dfc
Fix type export
jzempel Jan 10, 2022
490d5dd
Update Button element component + subcomponent type conventions
jzempel Jan 10, 2022
0ddd5f5
Update Colorpicker element component + subcomponent type conventions
jzempel Jan 10, 2022
0f76ecf
Fix circular dependency
jzempel Jan 11, 2022
948b4ce
Update Datepicker element component + subcomponent type conventions
jzempel Jan 11, 2022
afe876c
Update Dropdown element component + subcomponent type conventions
jzempel Jan 12, 2022
2d38efa
Fix dropdown Field testing
jzempel Jan 12, 2022
20bb8fe
Update forms element component + subcomponent type conventions
jzempel Jan 12, 2022
c9f7464
Update loaders element component + subcomponent type conventions
jzempel Jan 12, 2022
80f00df
Update modals element component + subcomponent type conventions
jzempel Jan 13, 2022
044bbe1
Update notification element component + subcomponent type conventions
jzempel Jan 13, 2022
ed6afc9
Update notification element component + subcomponent type conventions
jzempel Jan 13, 2022
201c72b
Update notification element component + subcomponent type conventions
jzempel Jan 13, 2022
336bb9e
Update pagination element component + subcomponent type conventions
jzempel Jan 13, 2022
c39055c
Update pagination element component + subcomponent type conventions
jzempel Jan 13, 2022
12bdff7
Update table element component + subcomponent type conventions
jzempel Jan 13, 2022
1ca4bf1
Update tabs element component + subcomponent type conventions
jzempel Jan 13, 2022
307f689
Update tags element component + subcomponent type conventions
jzempel Jan 13, 2022
446a9e9
Update theming element component + subcomponent type conventions
jzempel Jan 13, 2022
fb084a5
Update tooltip element component + subcomponent type conventions
jzempel Jan 13, 2022
96d896b
Update typography element component + subcomponent type conventions
jzempel Jan 13, 2022
47623c3
Remove forwardRef for cloned icons
jzempel Jan 18, 2022
b946422
Update template subcomponent structure
jzempel Jan 18, 2022
aacc24c
Update chrome element component + subcomponent type conventions
jzempel Jan 18, 2022
f4c3881
Correct typing
jzempel Jan 19, 2022
5782dad
Document element component and subcomponent rules and guidelines
jzempel Jan 21, 2022
dbdf6ea
Merge branch 'main' into jzempel/element-types
jzempel Jan 21, 2022
0296143
Address comments from @Francois-Esquire
jzempel Jan 24, 2022
f06df5d
Address @hzhu copy nit
jzempel Jan 24, 2022
102c90d
Merge branch 'main' into jzempel/element-types
jzempel Jan 24, 2022
4b96955
Address @Francois-Esquire documentation comment
jzempel Jan 25, 2022
deff0e5
Remove element extension props
jzempel Jan 25, 2022
4107c0a
Merge branch 'main' into jzempel/element-types
jzempel Jan 26, 2022
db114b2
Merge branch 'main' into jzempel/element-types
jzempel Jan 26, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 148 additions & 1 deletion docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
The API for Garden components is based on a Container-View-Element
architecture. This architecture provides a separation of concerns, advancing
[development](development.md) with an approach that is repeatable,
consistent, and reliable. The component parts of the architecture are explained in more detail below.
consistent, and reliable. The component parts of the architecture are explained
in more detail below. Use the `yarn new`
[command](development.md#package-creation) to generate starter code that
satisfies API rules for Garden components.

## Container components

Expand Down Expand Up @@ -85,3 +88,147 @@ components are exported as the public interface for Garden.
steer clear from DOM attribute naming conflicts.
- Exceptions to boolean naming are props that map directly to HTML attributes
such as `disabled` or `hidden`.
- Standard element JSDoc follows a strict set of rules outlined under the
[Garden documentation](documentation.md) guidelines

### Structures

The following annotated structures outline the basic code format for simple,
subcomponent, and complex elements.

#### Simple

The simplest form of a Garden element component is annotated below.

```tsx
/**
* Copyright Zendesk, Inc.
*
* Use of this source code is governed under the Apache License, Version 2.0
* found at http://www.apache.org/licenses/LICENSE-2.0.
*/

import React, { HTMLAttributes, forwardRef } from 'react';

/**
* @extends HTMLAttributes<HTMLElement>
*/
/* [1] */ export const SimpleComponent = forwardRef<HTMLAttributes<HTMLElement>>((props, ref) => (
<StyledSimpleElement ref={ref} {...props} />
));

/* [2] */ SimpleComponent.displayName = 'SimpleComponent';
```

1. Always add the `@extends` JSDoc to the export. The clause is extracted for
website API [documentation](documentation.md).
2. The `displayName` definition should be set to the exported component name and
satisfies linter checks for runtime naming

#### Subcomponent

The `SubElement` imported into the [complex example](#complex) below is
structured here along with annotation that highlights the difference in
structure.

```tsx
/**
* Copyright Zendesk, Inc.
*
* Use of this source code is governed under the Apache License, Version 2.0
* found at http://www.apache.org/licenses/LICENSE-2.0.
*/

import React, { HTMLAttributes, forwardRef } from 'react';
import PropTypes from 'prop-types';
import { StyledSubElement } from '../styled';

/* [1] */ export interface IElementSubElementProps extends HTMLAttributes<HTMLElement> {
/** Applies regular (non-bold) font weight */
isRegular?: boolean;
}

/* [2] */ const SubElementComponent = forwardRef<IElementSubElementProps>((props, ref) => (
<StyledSubElement ref={ref} {...props} />
));

/* [3] */ SubElementComponent.displayName = 'Element.SubElement';

/* [4] */ SubElementComponent.propTypes = {
isRegular: PropTypes.bool
};

/**
* @extends HTMLAttributes<HTMLElement>
*/
/* [5] */ export const SubElement = SubElementComponent;
Francois-Esquire marked this conversation as resolved.
Show resolved Hide resolved
```

1. The exported interface combines the name of the complex parent element
together with the `SubElement`
2. The functional component uses _XxxComponent_ naming. The `const` is not
exported due to a `displayName` [issue with
Storybook](https://github.com/storybookjs/storybook/issues/12263#issuecomment-1008870685).
3. Use the `Element.SubElement` string construct for consistent subcomponent
`displayName` naming
4. All API props should be set as `PropTypes` for runtime error checking
5. The `@extends` clause is always applied to the export. This export
hack-around allows subcomponent props to display in Storybook.

#### Complex

The following typed structure represents the layout for complex Garden
components – those with subcomponents as static properties. The annotations
listed below document areas of interest.

```tsx
/**
* Copyright Zendesk, Inc.
*
* Use of this source code is governed under the Apache License, Version 2.0
* found at http://www.apache.org/licenses/LICENSE-2.0.
*/

import React, { HTMLAttributes, forwardRef } from 'react';
import PropTypes from 'prop-types';
/* [1] */ import { SubElement } from './SubElement';
import { StyledElement } from '../styled';

export interface IElementProps extends HTMLAttributes<HTMLElement> {
/** Applies compact styling */
isCompact?: boolean;
}

/* [2] */ const ElementComponent = forwardRef<HTMLElement, IElementProps>((props, ref) => (
<StyledElement ref={ref} {...props} />
));

/* [3] */ ElementComponent.displayName = 'Element';

/* [4] */ ElementComponent.propTypes = {
isCompact: PropTypes.bool
};

/**
* @extends HTMLAttributes<HTMLElement>
*/
/* [5] */ export const Element = ElementComponent as typeof ElementComponent & {
SubElement: typeof SubElement;
};

/* [6] */ Element.SubElement = SubElement;
```

1. Subcomponents should each reside in separate files. In Garden, one component
= one file
2. The non-exported `const` is a React functional component and uses _XxxComponent_
naming style
3. The `displayName` definition should be set to the exported component name and
satisfies linter checks for runtime naming
Comment on lines +226 to +227
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@jzempel jzempel Jan 24, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

4. All API props should be set as `PropTypes` for runtime error checking
5. The component export provides type assertion that leverages the parent
component type and appends subcomponent type information. Always remember to add
the `@extends` JSDoc to be extracted for website API
[documentation](documentation.md).
6. After the type has been defined, subcomponents are exposed via static
property notation
47 changes: 47 additions & 0 deletions docs/documentation.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# Garden documentation

The JSDoc applied to exported [element components](api.md#element-components) is
extracted during the [website](https://github.com/zendeskgarden/website) build
process and used to generate API prop tables seen on component pages. Therefore,
Garden adheres to a strict set of rules for element component documentation.
Other additional component documentation is generally left to developer
discretion, but should honor Garden [content
guidelines](https://garden.zendesk.com/content). The following list enumerates
the standard rules for Garden element component documentation:

- All element-level components **must** be embellished with the `@extends` JSDoc
tag. These tags will indicate the HTMLElement interface of the element rendered
by the component.
- All element component props **must** be documented
- Props will be consistently documented using [present
simple](https://garden.zendesk.com/content/grammar#tense), [third
person](https://garden.zendesk.com/content/grammar#pronouns)
(understood) singular grammar. Prefer active vs. passive voice.
- A good mechanism for formulation is to think _“it \_\_\_\_\_”_ where “it”
refers to the prop being described and the filled-in blank becomes the prop’s
description
- Limited exceptions to this rule may be considered on a case-by-case basis
where the grammar construct feels clunky
- Do & don't examples:
- :white_check_mark: “Specifies the font size”
- :no_entry_sign: “Specify the font size”
- :white_check_mark: “Determines light mode styling”
- :no_entry_sign: “Whether light mode styling is used”
- :white_check_mark: “Applies danger styling” (active)
- :no_entry_sign: “Determines if danger styling is used” (passive)
- Function-type props _must_ document all `@param` values and any relevant
`@returns` value
- Use lowercase for component names referenced within prop descriptions
- Consider opportunities for adding internal cross-reference or external URL
links
- See the [Multiselect
API](https://garden.zendesk.com/components/multiselect#multiselect)
`renderItem` prop for an example of an internal cross-reference
- See the [Dropdown
API](https://garden.zendesk.com/components/select#dropdown) for an
external linking example
- 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.
Use this tag sparingly to hide internal-only APIs.
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@
import React, { HTMLAttributes, forwardRef } from 'react';
import { Styled{{capitalize component}}Text } from '../styled';

/**
* @extends HTMLAttributes<HTMLElement>
*/
export const {{capitalize component}}Text = forwardRef<HTMLElement, HTMLAttributes<HTMLElement>>((props, ref) => (
const {{capitalize component}}TextComponent = forwardRef<HTMLElement, HTMLAttributes<HTMLElement>>((props, ref) => (
Francois-Esquire marked this conversation as resolved.
Show resolved Hide resolved
<Styled{{capitalize component}}Text ref={ref} {...props} />
));

{{capitalize component}}Text.displayName = '{{capitalize component}}.Text';
{{capitalize component}}TextComponent.displayName = '{{capitalize component}}.Text';

/**
* @extends HTMLAttributes<HTMLElement>
*/
export const {{capitalize component}}Text = {{capitalize component}}TextComponent;
16 changes: 8 additions & 8 deletions packages/accordions/.size-snapshot.json
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
{
"index.cjs.js": {
"bundled": 46477,
"minified": 32456,
"gzipped": 6924
"bundled": 47240,
"minified": 33152,
"gzipped": 7002
},
"index.esm.js": {
"bundled": 43287,
"minified": 29747,
"gzipped": 6690,
"bundled": 44050,
"minified": 30443,
"gzipped": 6770,
"treeshaked": {
"rollup": {
"code": 24127,
"code": 24304,
"import_statements": 627
},
"webpack": {
"code": 27680
"code": 27855
}
}
}
Expand Down
50 changes: 20 additions & 30 deletions packages/accordions/src/elements/accordion/Accordion.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,7 @@
* found at http://www.apache.org/licenses/LICENSE-2.0.
*/

import React, {
useRef,
useEffect,
forwardRef,
RefAttributes,
HTMLAttributes,
PropsWithoutRef,
ForwardRefExoticComponent,
useMemo
} from 'react';
import React, { useRef, useEffect, forwardRef, HTMLAttributes, useMemo } from 'react';
import { useAccordion } from '@zendeskgarden/container-accordion';
import { StyledAccordion } from '../../styled';
import { AccordionContext } from '../../utils';
Expand All @@ -23,14 +14,6 @@ import { Header } from '../accordion/components/Header';
import { Label } from '../accordion/components/Label';
import { Panel } from '../accordion/components/Panel';

interface IStaticAccordionExport<T, P>
extends ForwardRefExoticComponent<PropsWithoutRef<P> & RefAttributes<T>> {
Section: typeof Section;
Header: typeof Header;
Label: typeof Label;
Panel: typeof Panel;
}

export interface IAccordionProps extends Omit<HTMLAttributes<HTMLDivElement>, 'onChange'> {
/** Sets `aria-level` heading rank in the document structure */
level: number;
Expand All @@ -56,10 +39,7 @@ export interface IAccordionProps extends Omit<HTMLAttributes<HTMLDivElement>, 'o
onChange?: (index: number) => void;
}

/**
* @extends HTMLAttributes<HTMLDivElement>
*/
export const Accordion = forwardRef<HTMLDivElement, IAccordionProps>(
const AccordionComponent = forwardRef<HTMLDivElement, IAccordionProps>(
(
{
level,
Expand Down Expand Up @@ -121,16 +101,11 @@ export const Accordion = forwardRef<HTMLDivElement, IAccordionProps>(
</AccordionContext.Provider>
);
}
) as IStaticAccordionExport<HTMLDivElement, IAccordionProps>;

Accordion.Section = Section;
Accordion.Header = Header;
Accordion.Label = Label;
Accordion.Panel = Panel;
);

Accordion.displayName = 'Accordion';
AccordionComponent.displayName = 'Accordion';

Accordion.defaultProps = {
AccordionComponent.defaultProps = {
isBare: false,
isCompact: false,
isAnimated: true,
Expand All @@ -139,3 +114,18 @@ Accordion.defaultProps = {
expandedSections: undefined,
onChange: () => undefined
};

/**
* @extends HTMLAttributes<HTMLDivElement>
*/
export const Accordion = AccordionComponent as typeof AccordionComponent & {
Header: typeof Header;
Label: typeof Label;
Panel: typeof Panel;
Section: typeof Section;
};

Accordion.Header = Header;
Accordion.Label = Label;
Accordion.Panel = Panel;
Accordion.Section = Section;
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import ChevronDown from '@zendeskgarden/svg-icons/src/16/chevron-down-stroke.svg
import { useAccordionContext, useSectionContext, HeaderContext } from '../../../utils';
import { StyledHeader, StyledRotateIcon, COMPONENT_ID as buttonGardenId } from '../../../styled';

export const Header = forwardRef<HTMLDivElement, HTMLAttributes<HTMLDivElement>>((props, ref) => {
const HeaderComponent = forwardRef<HTMLDivElement, HTMLAttributes<HTMLDivElement>>((props, ref) => {
const {
level: ariaLevel,
isCompact,
Expand Down Expand Up @@ -94,4 +94,9 @@ export const Header = forwardRef<HTMLDivElement, HTMLAttributes<HTMLDivElement>>
);
});

Header.displayName = 'Header';
HeaderComponent.displayName = 'Accordion.Header';

/**
* @extends HTMLAttributes<HTMLDivElement>
*/
export const Header = HeaderComponent;
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import React, { forwardRef, ButtonHTMLAttributes } from 'react';
import { StyledButton } from '../../../styled';
import { useAccordionContext, useHeaderContext, useSectionContext } from '../../../utils';

export const Label = forwardRef<HTMLButtonElement, ButtonHTMLAttributes<HTMLButtonElement>>(
const LabelComponent = forwardRef<HTMLButtonElement, ButtonHTMLAttributes<HTMLButtonElement>>(
(props, ref) => {
const sectionIndex = useSectionContext();
const { isCompact, isCollapsible, expandedSections } = useAccordionContext();
Expand All @@ -30,4 +30,9 @@ export const Label = forwardRef<HTMLButtonElement, ButtonHTMLAttributes<HTMLButt
}
);

Label.displayName = 'Label';
LabelComponent.displayName = 'Accordion.Label';

/**
* @extends ButtonHTMLAttributes<HTMLButtonElement>
*/
export const Label = LabelComponent;
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { StyledPanel, StyledInnerPanel } from '../../../styled';

type PanelProps = IAccordionContext | { isExpanded?: boolean };

export const Panel = forwardRef<HTMLElement, HTMLAttributes<HTMLElement>>((props, ref) => {
const PanelComponent = forwardRef<HTMLElement, HTMLAttributes<HTMLElement>>((props, ref) => {
const { isCompact, isBare, isAnimated, getPanelProps, expandedSections } = useAccordionContext();
const panelRef = useRef<HTMLElement>();
const index = useSectionContext();
Expand Down Expand Up @@ -63,4 +63,9 @@ export const Panel = forwardRef<HTMLElement, HTMLAttributes<HTMLElement>>((props
);
});

Panel.displayName = 'Panel';
PanelComponent.displayName = 'Accordion.Panel';

/**
* @extends HTMLAttributes<HTMLElement>
*/
export const Panel = PanelComponent;
Loading