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

[Tabs2] Add 'large' prop #1510

Merged
merged 1 commit into from
Aug 30, 2017
Merged

[Tabs2] Add 'large' prop #1510

merged 1 commit into from
Aug 30, 2017

Conversation

cmslewis
Copy link
Contributor

@cmslewis cmslewis commented Aug 28, 2017

Fixes #1506

Checklist

  • Include tests
  • Update documentation (automatically with new prop)

Changes proposed in this pull request:

Reviewers should focus on:

  • Prop description language. I took inspiration from NumericInput's large-prop description.
  • Should we cut a patch release (1.26.1)?

@blueprint-bot
Copy link

Add 'large' prop to Tabs2 + tests

Preview: documentation
Coverage: core | datetime

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

@cmslewis did you consider simply changing the CSS selector to support the old usage?

@cmslewis
Copy link
Contributor Author

@giladgray I figured a proper JS boolean approach would be a nice-to-have over a CSS class that you have to apply explicitly (we've discussed that in the past in #992). I can try a CSS-only change if that sounds better though.

@giladgray
Copy link
Contributor

I like the prop approach too, was just wondering if it was worth redoing the CSS change that caused this issue in the first place. But I'm down for what you've done here 👍

@cmslewis
Copy link
Contributor Author

Gotcha. It's possible a CSS approach would fix the issue here, but I figure it's nice to have this JS approach available anyway, so might as well just run with this for now.

@cmslewis cmslewis merged commit 1d9174e into master Aug 30, 2017
@cmslewis cmslewis deleted the cl/fix-large-tabs2 branch August 30, 2017 16:30
@cmslewis cmslewis mentioned this pull request Aug 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants