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 tag input #2086

Merged
merged 3 commits into from
Feb 7, 2018
Merged

flex tag input #2086

merged 3 commits into from
Feb 7, 2018

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Feb 5, 2018

Fixes #1425 (last remaining TODO)

  • actually three-column layout, with tags (and children) in center container
  • rightElement supports buttons and spinners out of the box, but other elements will likely need custom CSS

Thoughts

  • ⚠️ aligning the rightElement is basically impossible to generalize 😢 . any thoughts about how to handle this?
    • the left icon and values are easy to center because they're owned by TagInput: we know size, position, etc about those.
      • check out the margins on the left icon — they’re very specific to center it properly
        and they change when large
    • but rightElement?: React.ReactNode we know nothing about!
      in the docs example it’s a small button but it could be anything, from a large button to an entire app-in-a-box
      • the align-items: flex-start fucks it all up cuz i don’t know what kind of margins to apply to “center” the element in the one-line case

tag-input-flex

- actually three-column layout, with tags (and children) in center container
- aligning the rightElement is basically impossible to generalize :sad:
@blueprint-bot
Copy link

flex tag input

Preview: documentation | landing | table

// essentially a min-width, cuz flex allows it to grow or shrink:
width: $pt-grid-size * 10;
line-height: $tag-height;
Copy link
Contributor

Choose a reason for hiding this comment

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

we hit a small regression here:
screen recording google chrome

line-height: $pt-grid-size * 2 fixes it -- 20px because there's 5px margin at the top and at the bottom

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sure makes sense 👍

@adidahiya
Copy link
Contributor

adidahiya commented Feb 6, 2018

aligning the rightElement is basically impossible to generalize

what's the goal here? to center it vertically? IMO it's nbd if it requires custom CSS for oddly-shaped rightElement values 🤷‍♂️

@giladgray
Copy link
Contributor Author

yeah it's gonna require custom CSS for oddly-shaped elements. i'm going to ensure that buttons are perfectly centered (a la input group) and that's it.

@llorca
Copy link
Contributor

llorca commented Feb 6, 2018

cool sounds good. for tag input and input groups, can you make sure the right button is flex-start as opposed to centered?

@giladgray
Copy link
Contributor Author

input groups can only be one line, so gonna keep it centered. tag inputs, for sure.

@giladgray
Copy link
Contributor Author

i'm actually not touching input group at all, and they don't even use flex. they use padding and position because the <input> needs to go around the left/right elements for the :focus state

@blueprint-bot
Copy link

pt-button-height-{size}() mixins, TagInput support spinners

Preview: documentation | landing | table

@giladgray
Copy link
Contributor Author

ready for review

@adidahiya
Copy link
Contributor

adidahiya commented Feb 7, 2018

can we do anything about this? use <Text> for placeholder to ellipsize?
image

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

otherwise lgtm

@giladgray
Copy link
Contributor Author

@adidahiya that's an actual <input placeholder /> so there's not much that can be done

@adidahiya adidahiya merged commit 0225a12 into develop Feb 7, 2018
@adidahiya adidahiya deleted the gg/flex-tag-input branch February 7, 2018 01:02
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.

4 participants