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 menu item #2075

Merged
merged 10 commits into from
Feb 5, 2018
42 changes: 42 additions & 0 deletions packages/core/src/common/_flex.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright 2018 Palantir Technologies, Inc. All rights reserved.
// Licensed under the terms of the LICENSE file distributed with this project.

// this element becomes a flex container in the given direction.
// supply `$margin` to put space between each child.
// supply `$inline: inline` to use `display: flex-inline`.
// customize `flex: 1 1` child selector with $fill.
@mixin pt-flex-container($direction: row, $margin: none, $inline: none, $fill: ".pt-fill") {
@if $inline == inline or $inline == true {
display: inline-flex;
} @else {
display: flex;
}
flex-direction: $direction;

> * {
flex: 0 0 auto;
}

> #{$fill} {
flex: 1 1 auto;
}

@if $margin != none {
@include pt-flex-margin($direction, $margin);
}
}

// applies margin along axis of direction between every direct child, with no margins on either end.
// $direction: row | column
// $margin: margin[-right|-bottom] value
@mixin pt-flex-margin($direction, $margin) {
// CSS API support
&::before,
> :not(:last-child) {
@if $direction == row {
margin-right: $margin;
} @else {
margin-bottom: $margin;
}
}
}
1 change: 1 addition & 0 deletions packages/core/src/common/_mixins.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the terms of the LICENSE file distributed with this project.

@import "colors";
@import "flex";

$pt-intent-colors: (
"primary": $pt-intent-primary,
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/components/_index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
@import "editable-text/editable-text";
@import "forms/index";
@import "hotkeys/hotkeys";
@import "icon/icons";
@import "icon/icon";
@import "menu/menu";
@import "navbar/navbar";
@import "non-ideal-state/non-ideal-state";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

vertical-align: middle;
// inherit text color by default
fill: currentColor;
Expand Down
6 changes: 4 additions & 2 deletions packages/core/src/components/menu/_common.scss
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,19 @@ $dark-menu-item-color-active: $dark-minimal-button-background-color-active !defa
// customize modifier classes with params.
// setting modifier to "" will generally apply it as default styles due to & selectors
@mixin menu-item($disabled-selector: ".pt-disabled", $hover-selector: ":hover") {
@include overflow-ellipsis();
display: block;
@include pt-flex-container(row, $menu-item-padding);
align-items: center;
border-radius: $menu-item-border-radius;
padding: $menu-item-padding;
text-decoration: none;
line-height: $pt-icon-size-standard;
color: inherit;
user-select: none;

&#{$hover-selector} {
background-color: $menu-item-color-hover;
cursor: pointer;
text-decoration: none;
}

&#{$disabled-selector} {
Expand Down
31 changes: 0 additions & 31 deletions packages/core/src/components/menu/_menu.scss
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,12 @@ Styleguide pt-menu
&::before {
// support pt-icon-* classes directly on this element
@include pt-icon();
float: left;
margin-right: $menu-item-padding;
}

&::before,
&::after {
color: $pt-icon-color;
}

.pt-icon {
color: $pt-icon-color;

&:first-child {
margin-right: $menu-item-padding;
}
}

.pt-menu-item-label {
Expand All @@ -95,7 +86,6 @@ Styleguide pt-menu
color: $pt-text-color-disabled !important;

&::before,
&::after,
.pt-icon,
.pt-menu-item-label {
color: $pt-icon-color-disabled !important;
Expand All @@ -113,13 +103,6 @@ Styleguide pt-menu
}
}

a.pt-menu-item {
&,
&:hover {
text-decoration: none;
}
}

button.pt-menu-item {
border: none;
background: none;
Expand All @@ -128,11 +111,6 @@ button.pt-menu-item {
font-family: inherit;
}

.pt-menu-item-label {
float: right;
margin-left: $menu-item-padding;
}

/*
Menu headers

Expand Down Expand Up @@ -170,7 +148,6 @@ Styleguide pt-menu.pt-menu-header
@include menu-item-intent($pt-dark-intent-text-colors);

&::before,
&::after,
.pt-icon {
color: $pt-dark-icon-color;
}
Expand All @@ -179,13 +156,6 @@ Styleguide pt-menu.pt-menu-header
color: $pt-dark-text-color-muted;
}

&:hover {
&::before,
&::after {
color: $white;
}
}

&.pt-active,
&:active {
background-color: $dark-menu-item-color-active;
Expand All @@ -197,7 +167,6 @@ Styleguide pt-menu.pt-menu-header
color: $pt-dark-text-color-disabled !important;

&::before,
&::after,
.pt-icon,
.pt-menu-item-label {
color: $pt-dark-icon-color-disabled !important;
Expand Down
18 changes: 0 additions & 18 deletions packages/core/src/components/menu/_submenu.scss
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,6 @@
// override very specific selector for popovers in button groups
// stylelint-disable-next-line declaration-no-important
float: none !important;

> .pt-menu-item {
padding-right: $pt-icon-size-standard + $pt-grid-size;

&::after {
@include pt-icon();
position: absolute;
right: $half-grid-size;
content: $pt-icon-caret-right;

// need several layers of selectors to make sure we're targeting only
// the top level of menu items in the current submenu.
// stylelint-disable-next-line
.pt-large & {
line-height: $pt-icon-size-large;
}
}
}
}

> .pt-popover-open > .pt-menu-item {
Expand Down
12 changes: 11 additions & 1 deletion packages/core/src/components/menu/menuItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ export interface IMenuItemProps extends IActionProps, ILinkProps {
*/
label?: string | JSX.Element;

/**
* Whether the text should be allowed to wrap to multiple lines.
* If `false`, text will be truncated with an ellipsis when it reaches `max-width`.
* @default false
*/
multiline?: boolean;
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 Author

@giladgray giladgray Feb 3, 2018

Choose a reason for hiding this comment

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

@llorca thoughts on the centered icon alignment when multiline? (see your screenshot in comment below)

Copy link
Contributor

Choose a reason for hiding this comment

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

good call, actually aligned at the top would be better in this case


/** Props to spread to `Popover`. Note that `content` and `minimal` cannot be changed. */
popoverProps?: Partial<IPopoverProps>;

Expand All @@ -45,6 +52,7 @@ export interface IMenuItemProps extends IActionProps, ILinkProps {
export class MenuItem extends AbstractPureComponent<IMenuItemProps> {
public static defaultProps: IMenuItemProps = {
disabled: false,
multiline: false,
popoverProps: {},
shouldDismissPopover: true,
text: "",
Expand All @@ -66,6 +74,7 @@ export class MenuItem extends AbstractPureComponent<IMenuItemProps> {
},
this.props.className,
);
const textClasses = classNames(Classes.FILL, { [Classes.TEXT_OVERFLOW_ELLIPSIS]: !this.props.multiline });

const target = (
<a
Expand All @@ -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>
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.

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:
image
image

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

{label && <span className={Classes.MENU_ITEM_LABEL}>{label}</span>}
{this.props.text}
{hasSubmenu && <Icon iconName="caret-right" />}
</a>
);

Expand Down
5 changes: 3 additions & 2 deletions packages/docs-app/src/examples/core-examples/menuExample.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { Classes, Icon, Menu, MenuDivider, MenuItem } from "@blueprintjs/core";
import { BaseExample } from "@blueprintjs/docs-theme";

export class MenuExample extends BaseExample<{}> {
public className = "docs-menu-example";
protected renderExample() {
return (
<div>
Expand Down Expand Up @@ -39,8 +40,8 @@ export class MenuExample extends BaseExample<{}> {
</MenuItem>
<MenuItem iconName="asterisk" text="Miscellaneous">
<MenuItem iconName="badge" text="Badge" />
<MenuItem iconName="book" text="Book" />
<MenuItem iconName="more" text="More">
<MenuItem iconName="book" text="Long items will truncate when they reach max-width" />
<MenuItem iconName="more" text="Look in here for even more items">
<MenuItem iconName="briefcase" text="Briefcase" />
<MenuItem iconName="calculator" text="Calculator" />
<MenuItem iconName="dollar" text="Dollar" />
Expand Down
2 changes: 1 addition & 1 deletion packages/docs-app/src/index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,5 @@
}

.docs-releases-menu .pt-menu {
width: 320px;
min-width: 270px;
}
4 changes: 4 additions & 0 deletions packages/docs-app/src/styles/_examples.scss
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,10 @@
justify-content: center;
}

.docs-menu-example .pt-menu {
max-width: 280px;
}

.docs-wiggle {
animation: docs-wiggle-rotate $pt-transition-duration $pt-transition-ease infinite;
}
Expand Down
9 changes: 7 additions & 2 deletions packages/docs-theme/src/styles/_navigator.scss
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,13 @@ $navigator-min-width: $pt-grid-size * 22;
overflow: auto;
}

.pt-menu-item:not(.pt-active) {
background: inherit;
.pt-menu-item {
flex-direction: column;
align-items: flex-start;

&:not(.pt-active) {
background: inherit;
}
}
}

Expand Down