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

Ripple button example docs #6579

Merged
merged 10 commits into from
Jun 26, 2024
Merged

Ripple button example docs #6579

merged 10 commits into from
Jun 26, 2024

Conversation

snowystinger
Copy link
Member

Closes

✅ 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:

@rspbot
Copy link

rspbot commented Jun 20, 2024

@rspbot
Copy link

rspbot commented Jun 20, 2024

@rspbot
Copy link

rspbot commented Jun 20, 2024

@rspbot
Copy link

rspbot commented Jun 20, 2024

reidbarber
reidbarber previously approved these changes Jun 20, 2024
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

Looks great!

yihuiliao
yihuiliao previously approved these changes Jun 20, 2024
@rspbot
Copy link

rspbot commented Jun 21, 2024

@snowystinger snowystinger dismissed stale reviews from yihuiliao and reidbarber via ebaadfc June 21, 2024 04:49
@rspbot
Copy link

rspbot commented Jun 21, 2024

<ExampleCard
url="../Button.html"
title="Button"
description="A ripple helps indicate where an interaction has taken place.">
Copy link
Member

Choose a reason for hiding this comment

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

Was this description here meant to be this or should it be "A button allows a user to perform an action"? like on the other pages?

Copy link
Member Author

@snowystinger snowystinger Jun 23, 2024

Choose a reason for hiding this comment

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

Ripples are pretty confined to decorative uses, so I opted for this description. I could be convinced otherwise though, that this example should be more focused on the button and the fact it has a ripple is incidental. But then I'd rename all of it and it might be harder to find.

Copy link
Member

Choose a reason for hiding this comment

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

Ah just to be clear, I'm fine with the current description being present on the cards in the index part of the docs aka this
image
I just found it bit inconsistent with the other component examples that the component list section had the button labeled with the same description: https://reactspectrum.blob.core.windows.net/reactspectrum/ebaadfce4afacdcbc7b7c4f146c7c377c2128120/docs/react-aria/examples/ripple-button.html#components.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

o, i totally missed that one, thanks, I'll update

@rspbot
Copy link

rspbot commented Jun 25, 2024

@rspbot
Copy link

rspbot commented Jun 25, 2024

@rspbot
Copy link

rspbot commented Jun 25, 2024

@rspbot
Copy link

rspbot commented Jun 25, 2024

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@snowystinger snowystinger merged commit a48d167 into main Jun 26, 2024
28 checks passed
@snowystinger snowystinger deleted the ripple-button-example-docs branch June 26, 2024 00:48
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.

None yet

5 participants