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

flex button #2078

Merged
merged 15 commits into from
Feb 5, 2018
Merged

flex button #2078

merged 15 commits into from
Feb 5, 2018

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Feb 3, 2018

Changes proposed in this pull request:

  • pt-button-base() is inline flex row
  • ⚠️ Button renders text and children inside .pt-fill container
    • vertical buttons automatically align left and push rightIcon to right side!
    • loading easily hides this container without needing to wrap children!
  • Button now supports multiple lines of text, but will only wrap if forced to by layout constraints (like max-width)
  • add alignText prop to Button and ButtonGroup (and examples)

Reviewers should focus on:

  • new align prop: play with it in both ButtonGroup JS examples

@blueprint-bot
Copy link

fix row margin when large, right icon spacing in CSS API

Preview: documentation | landing | table

* to the appropriate side. `icon` and `rightIcon` will be pushed to either side.
* @default "center"
*/
align?: "left" | "center" | "right";
Copy link
Contributor

Choose a reason for hiding this comment

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

😍 😍 😍

Copy link
Contributor

Choose a reason for hiding this comment

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

this conflicts somewhat with the NavbarGroup align prop -- here, you are referring to the alignment of content while for NavbarGroup (as well as most design tools), align refers to the component's alignment in its container. I suggest renaming this to textAlignment or alignText

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm yes good catch @adidahiya! i'll go with alignText

[Classes.LARGE]: large,
[Classes.MINIMAL]: minimal,
Copy link
Contributor

Choose a reason for hiding this comment

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

small?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would prefer not to add tangential new props in this PR.
i plan to add modifier props across the board after 2.0 (non-breaking)

fill: false,
iconOnly: false,
large: false,
minimal: false,
Copy link
Contributor

@llorca llorca Feb 3, 2018

Choose a reason for hiding this comment

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

small modifier in examples would be neat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ButtonGroup doesn't have a small prop.
let's wait on that, i want to add modifier props everywhere

Copy link
Contributor

@llorca llorca left a comment

Choose a reason for hiding this comment

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

🔥 🔥

spotted a couple of problems with buttons in input groups:

  • regular size has an icon size issue:

screen shot 2018-02-02 at 7 33 23 pm

  • large size has both an icon size and height issue:

screen shot 2018-02-02 at 7 33 20 pm

@blueprint-bot
Copy link

let flex figure out heights of numeric-input buttons 👌

Preview: documentation | landing | table

* to the appropriate side. `icon` and `rightIcon` will be pushed to either side.
* @default "center"
*/
align?: "left" | "center" | "right";
Copy link
Contributor

Choose a reason for hiding this comment

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

this conflicts somewhat with the NavbarGroup align prop -- here, you are referring to the alignment of content while for NavbarGroup (as well as most design tools), align refers to the component's alignment in its container. I suggest renaming this to textAlignment or alignText

@adidahiya
Copy link
Contributor

otherwise lgtm

@blueprint-bot
Copy link

align => alignText, add more button classes

Preview: documentation | landing | table

@giladgray giladgray changed the base branch from flex-containers to develop February 5, 2018 21:42
@blueprint-bot
Copy link

Merge branch 'develop' of github.com:palantir/blueprint into gg/flex-button

Preview: documentation | landing | table

@giladgray giladgray merged commit bbb3da4 into develop Feb 5, 2018
@giladgray giladgray deleted the gg/flex-button branch February 5, 2018 21:48
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.

4 participants