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

Icon: new docs #1698

Merged
merged 2 commits into from Sep 23, 2021
Merged

Icon: new docs #1698

merged 2 commits into from Sep 23, 2021

Conversation

AlbertCarreras
Copy link
Contributor

@AlbertCarreras AlbertCarreras commented Sep 20, 2021 •
edited

Summary

Icon: new docs + Iconography and svgs guidelines

Links

  • [Jira](link to Jira ticket(s))
  • [TDD](link to Paper doc)
  • [Figma](link to Figma file)

Checklist

  • Added unit and Flow Tests
  • Added documentation + accessibility tests
  • Verified accessibility: keyboard & screen reader interaction
  • Checked dark mode, responsiveness, and right-to-left support
  • Checked stakeholder feedback (e.g. Gestalt designers)

@AlbertCarreras AlbertCarreras requested a review from a team as a code owner September 20, 2021 17:34
@AlbertCarreras AlbertCarreras added the patch release Patch release label Sep 20, 2021
@AlbertCarreras AlbertCarreras changed the title Icon: new docs WIP Icon: new docs Sep 20, 2021
@netlify
Copy link

netlify bot commented Sep 20, 2021 •
edited

✔️ Deploy Preview for gestalt ready!

🔨 Explore the source changes: 0022060

🔍 Inspect the deploy log: https://app.netlify.com/sites/gestalt/deploys/614cb38a23658f0007485991

😎 Browse the preview: https://deploy-preview-1698--gestalt.netlify.app

christianvuerings
christianvuerings reviewed Sep 21, 2021
View reviewed changes
docs/pages/icon.js Outdated
return <CardPage cards={cards} page="Icon" />;
export async function getStaticProps(): Promise<{| props: {| generatedDocGen: DocGen |} |}> {
return {
props: { generatedDocGen: await docgen('Icon') },
Copy link
Contributor

Choose a reason for hiding this comment

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

@AlbertCarreras a proposal to make the icon type on Icon work: christianvuerings@71faa1d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

@AlbertCarreras AlbertCarreras changed the title WIP Icon: new docs Icon: new docs Sep 22, 2021
dangerismycat
dangerismycat reviewed Sep 23, 2021
View reviewed changes
cypress/integration/accessibility_Icon_spec.js Outdated
cy.configureAxe({
rules: [
{
id: 'aria-command-name',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we fix the violations instead of disabling the rule? If not, can you add a comment here about the issue(s)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll document it better. The problem is that sometimes the cmp has an empty label because there's a wrapper giving it a name, such as a tooltip. And I think this error come from there.

cypress/integration/accessibility_Iconography and SVGs_spec.js Outdated
cy.injectAxe();
});

// Disable the test for now since it's timing out on GitHub CI
Copy link
Contributor

Choose a reason for hiding this comment

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

Bummer. Do you have a strategy to get it working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy pasting bad... cleaning up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

packages/gestalt/src/AvatarGroup.js Outdated
*
* See the [Accessibility guidelines](https://gestalt.pinterest.systems/AvatarGroup#Accessibility) for details on proper usage.
* See the [Accessibility](#ARIA-attributes) guidelines for details on proper usage.
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a regression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. no idea how this got changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

packages/gestalt/src/AvatarGroup.js Outdated
@@ -16,9 +16,9 @@ type Size = 'xs' | 'sm' | 'md' | 'fit';
type UnionRefs = HTMLDivElement | HTMLAnchorElement;
type Props = {|
/**
* Label for the component used for screen readers.
* Label for screen readers to announce Icon.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does AvatarGroup show icons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I liked better the other description. Missed changing icon with avatargroup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

@AlbertCarreras AlbertCarreras merged commit bb73fe2 into pinterest:master Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christianvuerings christianvuerings christianvuerings left review comments

@dangerismycat dangerismycat dangerismycat approved these changes

Assignees
No one assigned
Labels
patch release Patch release
Projects
None yet
Milestone
No milestone
3 participants
@AlbertCarreras @christianvuerings @dangerismycat