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.

Sign up for GitHub

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

[Table] Refactor header menus to divorce click-target size from gradient size #906

Merged
merged 3 commits into from
Apr 3, 2017
Merged

[Table] Refactor header menus to divorce click-target size from gradient size #906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8e5f1a9
Refactor header menus to divorce click-target size from gradient size
cmslewis Mar 29, 2017
28409d6
Fix dark theme bg color
cmslewis Apr 3, 2017
2c04e51
Fix interaction bar style bugs
cmslewis Apr 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/table/src/common/classes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ export const TABLE_ROW_NAME_TEXT = "bp-table-row-name-text";
export const TABLE_SELECTION_ENABLED = "bp-table-selection-enabled";
export const TABLE_SELECTION_REGION = "bp-table-selection-region";
export const TABLE_TH_MENU = "bp-table-th-menu";
export const TABLE_TH_MENU_CONTAINER = "bp-table-th-menu-container";
export const TABLE_TH_MENU_CONTAINER_BACKGROUND = "bp-table-th-menu-container-background";
export const TABLE_THEAD = "bp-table-thead";
export const TABLE_TOP_CONTAINER = "bp-table-top-container";
export const TABLE_TRUNCATED_CELL = "bp-table-truncated-cell";
Expand Down
80 changes: 53 additions & 27 deletions packages/table/src/headers/_headers.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,24 @@
}
}

.bp-table-th-menu {
.bp-table-th-menu-container {
flex-shrink: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't need flex properties if you're using position: absolute but whatever

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i wasn't actually sure what this rule was doing (i'm no flex expert), but it was there before, so i left it in the refactor.

position: absolute;
right: $cell-border-width; // don't overlap box-shadow border
opacity: 0; // hide until header is hovered
text-align: right;

.bp-table-header:hover &,
.bp-table-header-active & {
opacity: 1;
}

.bp-table-interaction-bar & {
line-height: $interaction-bar-height;
}
}

.bp-table-th-menu-container-background {
@function menu-background($color) {
@return linear-gradient(90deg, rgba($color, 0) 0%, $color 50%);
}
Expand All @@ -65,18 +82,39 @@
mix($pt-intent-primary, $dark-header-hover-background-color, 10%)
);

flex-shrink: 0;
position: absolute;
top: 0;
right: $cell-border-width; // don't overlap box-shadow border
opacity: 0; // hide until header is hovered
background: none;
right: 0;
width: $column-header-min-height + $pt-grid-size * 2;
height: $column-header-min-height;
pointer-events: none; // don't swallow clicks meant for the underlying header cell

.bp-table-header:hover &,
.bp-table-header-active & {
background-image: $menu-hover-background;

.pt-dark & {
background-image: $dark-menu-hover-background;
}
}

.bp-table-header.bp-table-header-selected & {
background-image: $menu-selected-background;

.pt-dark & {
background-image: $dark-menu-selected-background;
}
}

.bp-table-interaction-bar & {
height: $interaction-bar-height;
}
}

.bp-table-th-menu {
cursor: $action-cursor;
width: $column-header-min-height + $pt-grid-size;
width: $column-header-min-height; // create a larger, more accessible click target
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think comment means much, and will mean even less in a month. it doesn't even look correct when i compare with code on the left, where the width was obviously larger.

Copy link
Contributor Author

@cmslewis cmslewis Mar 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

fair, though in general i like quick little CSS comments explaining the intent behind a rule. regardless of the target size prior to this refactor, this element's purpose is still to create a larger click target than the 16px icon it wraps, so the comment seemed helpful. if the intent is obvious, happy to remove.

height: $column-header-min-height;
padding-right: $pt-grid-size / 2 + $cell-border-width * 2; // icon has a border
text-align: right;
pointer-events: all;

.bp-table-interaction-bar & {
right: $cell-border-width;
Expand All @@ -94,12 +132,18 @@
}

.pt-icon-standard {
$icon-offset: ($column-header-min-height - $pt-icon-size-standard) / 2;

margin-top: $icon-offset;
margin-right: $icon-offset;
border-radius: $pt-border-radius;
box-shadow: inset 0 0 0 1px rgba($black, $pt-drop-shadow-opacity);
background-color: $header-hover-background-color;
color: $pt-icon-color;

.pt-dark & {
box-shadow: inset 0 0 0 1px rgba($white, $pt-drop-shadow-opacity);
background-color: $dark-header-hover-background-color;
color: $pt-dark-icon-color;
}
}
Expand All @@ -125,24 +169,6 @@
color: $white;
}
}

.bp-table-header:hover &,
.bp-table-header-active & {
opacity: 1;
background-image: $menu-hover-background;

.pt-dark & {
background-image: $dark-menu-hover-background;
}
}

.bp-table-header.bp-table-header-selected & {
background-image: $menu-selected-background;

.pt-dark & {
background-image: $dark-menu-selected-background;
}
}
}

.bp-table-thead {
Expand Down
25 changes: 14 additions & 11 deletions packages/table/src/headers/columnHeaderCell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,17 +209,20 @@ export class ColumnHeaderCell extends React.Component<IColumnHeaderCellProps, IC
}];

return (
<Popover
tetherOptions={{ constraints }}
content={menu}
position={Position.BOTTOM}
className={Classes.TABLE_TH_MENU}
popoverDidOpen={this.getPopoverStateChangeHandler(true)}
popoverWillClose={this.getPopoverStateChangeHandler(false)}
useSmartArrowPositioning={true}
>
<span className={popoverTargetClasses}/>
</Popover>
<div className={Classes.TABLE_TH_MENU_CONTAINER}>
<div className={Classes.TABLE_TH_MENU_CONTAINER_BACKGROUND} />
<Popover
tetherOptions={{ constraints }}
content={menu}
position={Position.BOTTOM}
className={Classes.TABLE_TH_MENU}
popoverDidOpen={this.getPopoverStateChangeHandler(true)}
popoverWillClose={this.getPopoverStateChangeHandler(false)}
useSmartArrowPositioning={true}
>
<span className={popoverTargetClasses}/>
</Popover>
</div>
);
}

Expand Down