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

[Tag] Clarify that onClick and onRemove are mutually exclusive #2987

Merged
merged 2 commits into from
Jan 29, 2021

Conversation

chloerice
Copy link
Member

@chloerice chloerice commented May 14, 2020

WHY are these changes introduced?

#2986 brought to my attention that there isn't a removable tag example, and that it needs to be documented that clickable tags aren't removable.

WHAT is this pull request doing?

This PR adds a removable tag example and clarifies that the remove button doesn't render when onClick is set.

***Note: The pa11y test failure is due to a bug reported with Autocomplete.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@github-actions
Copy link
Contributor

github-actions bot commented May 14, 2020

🟢 This pull request modifies 3 files and might impact 3 other files.

Details:
All files potentially affected (total: 3)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

📄 src/components/Tag/README.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Tag/Tag.tsx (total: 3)

Files potentially affected (total: 3)

@chloerice chloerice force-pushed the improve-tag-docs branch 2 times, most recently from d3337b3 to 95d933b Compare May 14, 2020 15:29
@danrosenthal
Copy link
Contributor

danrosenthal commented May 14, 2020

Would it be useful to actually remove the tag in the example?

@chloerice
Copy link
Member Author

Would it be important to actually remove the tag in the example?

It's moreso illustrating that setting onRemove adds a remove button you can wire up. But we do have a simple tag example in the Autocomplete docs I can copy over!

@BPScott
Copy link
Member

BPScott commented May 14, 2020

Could we also help enforce this in the Typescript declaration of Tag's props? TextField does similar where you can have only one of readonly, disabled or onChange

@chloerice
Copy link
Member Author

Could we also help enforce this in the Typescript declaration of Tag's props? TextField does similar where you can have only one of readonly, disabled or onChange

Marc did that for Tag as well.

@BPScott
Copy link
Member

BPScott commented May 14, 2020

Cool, ignore me :)

@BPScott
Copy link
Member

BPScott commented May 14, 2020

It feels a little odd for the demo of removable tag to not show any on initial render, but to instead show an autocomplete box.Doesn't fee very useful for our VRT stuff.

How about something like this as an example:

function RemovableTagExample() {
  const [selectedTags, setSelectedTags] = useState([
    'Rustic',
    'Antique',
    'Vinyl',
    'Refurbished',
  ]);

  const removeTag = useCallback(
    (tag) => () => {
      setSelectedTags((previousTags) =>
        previousTags.filter((previousTag) => previousTag !== tag),
      );
    },
    [],
  );

  const tagMarkup = selectedTags.map((option) => (
    <Tag key={option} onRemove={removeTag(option)}>
      {option}
    </Tag>
  ));

  return <Stack spacing="tight">{tagMarkup}</Stack>;
}

Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Code looks good to me 🥇 Thank Chloe!

@chloerice
Copy link
Member Author

@kyledurand Would you mind merging this for me? I don't have admin privileges and the pa11y failures don't have anything to do with this PR (the autocomplete is setting aria-expanded on the text field instead of the combobox wrapper, see #3639).

@kyledurand
Copy link
Contributor

I can merge for you but I'd prefer if you could add those two failing ids to our accessibility checks list https://github.com/Shopify/polaris-react/blob/master/scripts/accessibility-check.js#L88

@alex-page alex-page force-pushed the master branch 2 times, most recently from 4eab9e5 to 9e09ba7 Compare January 15, 2021 05:17
Base automatically changed from master to main January 21, 2021 15:38
@BPScott BPScott force-pushed the improve-tag-docs branch 3 times, most recently from c60ccba to b222df9 Compare January 29, 2021 18:21
@BPScott
Copy link
Member

BPScott commented Jan 29, 2021

No need to fixup a11y tests skip lists if we've already fixed all our failing a11y tests :thonk:

I've updated the removable example per my above comment so that it demos tags immediately rather than needing you to interact with an autocomplete

@BPScott BPScott merged commit eb0bac1 into main Jan 29, 2021
@BPScott BPScott deleted the improve-tag-docs branch January 29, 2021 18:44
sylvhama pushed a commit that referenced this pull request Mar 26, 2021
* [Tag] Clarify that onClick and onRemove are mutually exclusive

* Shorter, clearer removable tag example

Co-authored-by: Ben Scott <ben.scott@shopify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants