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

Improve badge for click tracking and links #268

Merged
merged 1 commit into from Oct 24, 2019
Merged

Improve badge for click tracking and links #268

merged 1 commit into from Oct 24, 2019

Conversation

miqh
Copy link
Contributor

@miqh miqh commented Oct 23, 2019

Hello,

These changes aim to address the improvements suggested in #173.

I'm aware n_clicks_timestamp wasn't requested, but I figure I'd include it for consistency since most other components supporting n_clicks also has the former.

I transplanted the documentation for the new prop types from existing component implementations.

I've also introduced (couldn't find an existing one) a test file for the badge component that covers the new behaviour introduced.

Copy link
Collaborator

@tcbegley tcbegley left a comment

Hey @miqh

Thanks so much for your contribution! Particularly for adding tests. I've somewhat neglected these to date as I've invested most of my time into improving documentation instead 😳 I definitely want to improve the state of the testing now that I'm mostly done with the docs for now.

I've made a couple of minor inline comments, really just for making this component consistent with other components that do similar things, specifically Button and DropdownMenuItem, and which I think helps readability a bit. If you are able to incorporate those I'd be happy to merge and include in the next release.

Thanks again for taking the time to contribute!

src/components/Badge.js Outdated Show resolved Hide resolved
src/components/Badge.js Outdated Show resolved Hide resolved
Extends the badge component to offer additional properties for tracking
click interactions. Links are also rendered appropriately.

Addresses #173
Copy link
Collaborator

@tcbegley tcbegley left a comment

This is great, thanks again for your contribution!

@tcbegley tcbegley merged commit cdf342f into facultyai:master Oct 24, 2019
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants