react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

Add icon only docs for Button

Open jluyau opened this issue 1 year ago โ€ข 9 comments

Closes

Add icon-only entry for Button in docs

โœ… Pull Request Checklist:

  • [ ] Included link to corresponding React Spectrum GitHub Issue.
  • [ ] Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • [ ] Filled out test instructions.
  • [ ] Updated documentation (if it already exists for this component).
  • [ ] Looked at the Accessibility Practices for this feature - Aria Practices

๐Ÿ“ Test Instructions:

๐Ÿงข Your Project:

jluyau avatar Sep 09 '22 03:09 jluyau

Noticed that the padding is somehow off on the Events example now: https://reactspectrum.blob.core.windows.net/reactspectrum/eab25a55e7e4b7b5895d0670e92202c95d6b5057/docs/react-spectrum/Button.html#events

devongovett avatar Sep 09 '22 17:09 devongovett

+1 to what @ktabors mentioned. I think a specific callout to the "If the label is hidden, an icon is required, and the label will appear in a tooltip" portion of the Spectrum guidelines is appropriate here in case people don't click on that guideline link. It would also be good to have the examples follow this practice as well.

LFDanLu avatar Sep 12 '22 18:09 LFDanLu

and the label will appear in a tooltip

this is not true of our implementation however.

devongovett avatar Sep 12 '22 18:09 devongovett

right, we currently rely on the user to provide the tooltip... Is the tooltip mandatory for icon only buttons? If so, we could wrap it in a TooltipTrigger for the user but we would need them to provide a tooltip text value through a different prop. If it isn't mandatory but strongly recommended IMO we should have the example display that. Otherwise we can keep it as is

LFDanLu avatar Sep 12 '22 18:09 LFDanLu

I wouldn't add Tooltip to Button. We could add an example though.

dannify avatar Sep 21 '22 21:09 dannify