flex menu item #2075
flex menu item #2075
Conversation
MenuExample demonstrates truncationPreview: documentation | landing | table |
| @@ -42,9 +42,6 @@ span.pt-icon { | |||
| svg.pt-icon { | |||
| // respect dimensions exactly | |||
| flex: 0 0 auto; | |||
| // SVG and DOM elements don't align perfectly - this magic number seems to fix it. | |||
| // https://blog.prototypr.io/align-svg-icons-to-text-and-say-goodbye-to-font-icons-d44b3d7b26b4#6e0c | |||
| margin-top: -0.125em; | |||
There was a problem hiding this comment.
align-items is much better at alignment than humans, so this shouldn't be necessary. keep an eye out for SVG icons mixed with text to ensure they're centered.
navigator uses flex columnm, adjust releases min-widthPreview: documentation | landing | table |
add `multiline` prop to disable this behavior.
giladgray
force-pushed
the
gg/flex-menu-item
branch
from
5cc1bb9
to
9f3f5e6
Compare
navigator uses flex columnm, adjust releases min-widthPreview: documentation | landing | table |
| * If `false`, text will be truncated with an ellipsis when it reaches `max-width`. | ||
| * @default false | ||
| */ | ||
| multiline?: boolean; |
There was a problem hiding this comment.
@llorca thoughts on the centered icon alignment when multiline? (see your screenshot in comment below)
There was a problem hiding this comment.
good call, actually aligned at the top would be better in this case
| @@ -76,8 +85,9 @@ export class MenuItem extends AbstractPureComponent<IMenuItemProps> { | |||
| target={this.props.target} | |||
| > | |||
| <Icon iconName={this.props.iconName} /> | |||
| <span className={textClasses}>{this.props.text}</span> | |||
There was a problem hiding this comment.
back to the topic of our public CSS API! this is very similar to what I had a few months ago--it was considered breaking then, so it's technically breaking now since you're adding a span. if you remove the span from this current PR, this is what it looks like:
reiterating that it's quite annoying, this sort of internal refactor should not be considered breaking, and we should be able to do these outside of major versions
There was a problem hiding this comment.
this implementation is not an API break, though! i added an element but only inside the React component. you can of course easily break the appearance by mucking in the console--that doesn't mean it's an API break.
you'll notice, most importantly, that all the CSS markup examples still look perfect, like before. I made no external changes to the CSS or React APIs.
folks who use only MenuItem and not .pt-menu-item will have a seamless upgrade experience (unless they have custom styles, of course).
|
@adidahiya browser tooltip would be nice. do you imagine it only appears when the text is ellipsized (requires measuring, though we do have the |
|
ideally it appears only when text is ellipsized (yes, requires measuring) this seems like a regression (currently, text doesn't get cut off like this), so we should block this PR on adding the tooltip |
|
|
|
|
use Text component for ellipsize + native tooltip behaviorPreview: documentation | landing | table |
Merge branch 'develop' of github.com:palantir/blueprint into gg/flex-menu-itemPreview: documentation | landing | table |
Fixes #984
Changes proposed in this pull request:
pt-flex-container($dir, $margin, $inline, $fill)mixin does it allmenu-item()is apt-flex-container(row)with margin between itemsMenuItemsubmenu caret in React (submenus are JS-only)MenuItemapplies ellipsis class automatically to text wrapper; can be disabled with newmultiline={true}propI don't think there are any API breaks here!?