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.

Sign up for GitHub

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

Jump to bottom

chore(chrome): update prop descriptions #954

Merged
merged 9 commits into from
Dec 17, 2020
Merged

chore(chrome): update prop descriptions #954

merged 9 commits into from
Dec 17, 2020

Conversation

Mikaelia
Copy link
Contributor

Description

Updates chrome package element prop docs

@Mikaelia Mikaelia requested a review from a team as a code owner December 16, 2020 00:07
@coveralls
Copy link

coveralls commented Dec 16, 2020 •
edited

Coverage Status

Coverage remained the same at 95.734% when pulling 5b813ae on mikaela/chrome into c46923f on main.

@zendesk-garden zendesk-garden temporarily deployed to staging December 16, 2020 00:40 Inactive
jzempel
jzempel requested changes Dec 16, 2020
View reviewed changes
packages/chrome/src/elements/Chrome.tsx Outdated Show resolved Hide resolved
@zendesk-garden zendesk-garden temporarily deployed to staging December 16, 2020 19:46 Inactive
jzempel
jzempel reviewed Dec 16, 2020
View reviewed changes
Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

Oops, @Mikaelia, I realized there are a bunch more top-level element-level components to document for chrome. See https://garden.zendesk.com/components/chrome#api for details. Were you planning to do those here or under separate PR?

@Mikaelia
Copy link
Contributor Author

Mikaelia commented Dec 16, 2020 •
edited

@jzempel Oops! Somehow I had it in my mind that I wasn't supposed to document SubNav. I'll get that fixed ASAP. Looking back at the code I do have a couple of questions though:

Footer -> I did not see any props to document
Header -> I see a prop isStandalone proptype but not the interface for it. Same with HeaderItemText for isClipped and SubNavItemText..are these props that I should still document?

This is the same with ChevronButton as well

@jzempel
Copy link
Member

jzempel commented Dec 16, 2020

Yes, there are a list of components that do not have any props. Header is unique in that it inherits its prop (and description) from StyledHeader. Same for the others you mentioned. So you'll need to dip into the styled directory in a few cases to spruce up the JSDoc. The trick is to find the origin for all component props listed under https://garden.zendesk.com/components/chrome#api (CollapsibleSubNavItem, etc).

@zendesk-garden zendesk-garden temporarily deployed to staging December 17, 2020 06:04 Inactive
jzempel
jzempel requested changes Dec 17, 2020
View reviewed changes
Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

Thanks for picking up these leftovers. Just a few more change requests from me.

packages/chrome/src/elements/subnav/CollapsibleSubNavItem.tsx Outdated
header?: React.ReactNode;
/** Reveals the item text */
Copy link
Member

Choose a reason for hiding this comment

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

Replace item text with something like "item's content" or "sub-items" (not sure that's a term we want to use 😝 ) or, similar to Accordion "item's section" (as this is technically an accordion control).

See the demo at https://garden.zendesk.com/components/chrome#navigation-panel with "show subnav" activated for additional working details for a CollapsibleSubNavItem.

packages/chrome/src/styled/header/StyledBaseHeaderItem.ts Outdated
/**
* Horizontally maximize a flex item in the header to take as much space as possible (i.e. breadcrumb container)
**/
/** Maximizes the width of a flex item in the header (i.e. breadcrumb container) */
Copy link
Member

Choose a reason for hiding this comment

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

We should remove (i.e. breadcrumb container) as design research has determined planting a breadcrumb in the header is off-pattern.

packages/chrome/src/styled/header/StyledBaseHeaderItem.ts Outdated
/**
* Round the border radius for a header item (i.e. user icon)
**/
/** Rounds the border radius of the item (i.e. user icon) */
Copy link
Member

Choose a reason for hiding this comment

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

Per guidance from Content Design and https://garden.zendesk.com/components/avatar#how-to-use-it-well, I wonder if we should swap user with "person". Or does "person icon" just sound a bit weird for dev docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need the bit inside the parenthesis? For me I'm not sure if it adds value as it seems pretty self-explanatory

Copy link
Member

Choose a reason for hiding this comment

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

Nope, good call.

packages/chrome/src/styled/header/StyledHeaderItemText.ts Outdated
/**
* Clip text (but leave accessible to screenreaders) for an icon-only header item
**/
/** Hides item text. Text remains accessible to screenreaders. */
Copy link
Member

Choose a reason for hiding this comment

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

screenreaders -> "screen readers". https://en.wikipedia.org/wiki/Screen_reader

packages/chrome/src/styled/header/StyledLogoHeaderItem.ts Outdated
/**
* Applies product-specific color palette
**/
/** Applies a product-specific color palette */
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 could be improved with "Applies a brand color to the product logo".

For the fussy details, the given name is used to dereference a brand color from the product portion of the PALETTE object. And then that color hex is inherited by currentColor used within one of the two-tone "relationshape" icons seen here: https://garden.zendesk.com/design/icons/26.

Copy link
Member

Choose a reason for hiding this comment

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

TBH, I regret having built this component 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, I see. Thanks for the explanation!

@zendesk-garden zendesk-garden temporarily deployed to staging December 17, 2020 19:39 Inactive
@zendesk-garden zendesk-garden temporarily deployed to staging December 17, 2020 20:50 Inactive
@jzempel jzempel merged commit 3c34e23 into main Dec 17, 2020
@jzempel jzempel deleted the mikaela/chrome branch December 17, 2020 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzempel jzempel jzempel approved these changes

@austingreendev austingreendev austingreendev approved these changes

Assignees
No one assigned
Labels
PR: Docs 📝
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Mikaelia @coveralls @jzempel @austingreendev @zendesk-garden