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

CloseButton #120

Closed
wants to merge 13 commits into from
Closed

CloseButton #120

wants to merge 13 commits into from

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Jul 10, 2018

This adds the CloseButton component to close #114, which I think means just the little ✖️ that lives in the corner of various modals and menus. We'll probably need to refactor this (and perhaps Button, too) a bit more as we put it to use in other components (for instance, SelectMenu in #116), but I think this is a good start.

I had to add support for a new value of the Button component's scheme prop: scheme="octicon", which translates to the btn-link text-inherit utility classes. I originally looked at using Box-btn-octicon, but that didn't work unless it was contained by a Box.

We could revisit this to use the btn-octicon class if we decide to take on primer/css#413.

}

CloseButton.propTypes = {
...Button.propTypes,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should call out the onClick prop here so that it's easier for people looking at the code to see that it's supported.

@shawnbot
Copy link
Contributor Author

We agreed in the component review that this is probably not the right way do the close button, because this particular component shouldn't support most of the props that Button does (children, disabled, linkStyle, etc.). It's also saddling the Button component with features that only a "subclass" uses, which has a pretty nasty code smell.

Instead, let's introduce an OcticonButton that renders a <button> with a required aria-label and an Octicon child, and can easily be used to make a close button.

@shawnbot shawnbot closed this Jul 11, 2018
@shawnbot shawnbot deleted the close-button branch July 11, 2018 22: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.

1 participant