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

Add option to 'pin' the menu to the screen as a fixed side panel #441

Merged
merged 4 commits into from
Feb 25, 2021

Conversation

phillipdupuis
Copy link
Contributor

@phillipdupuis phillipdupuis commented Feb 20, 2021

Certain workflows require frequent menu access (ex: if you are fiddling around with custom filters and need to go through a lot of trial & error to get them just right), so this is just a small QoL update to reduce clicks. It's also a nice option for new users, since it makes it easy to see all the available functionality while they're testing it out

(I'm sure I'll need to make some tweaks, I'm just getting the ball rolling)

I also haven't been able to test out the build, my circleci setup seems to be busted and I can't get any new branches to show up..hm

Copy link
Collaborator

@aschonfeld aschonfeld left a comment

Choose a reason for hiding this comment

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

Looks good so far. I think you need to make one small change and I left some questions as well.

@@ -21,35 +22,32 @@ function renderDimensionSelection(dimensionSelection) {
class ReactXArrayOption extends React.Component {
constructor(props) {
super(props);
this.buttonRef = React.createRef();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need this buttonRef isn't that all handled within MenuItem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100% right, hat's a mistake that I forgot to remove :) will get that

@@ -17,7 +18,7 @@ class ReactThemeOption extends React.Component {
const { setTheme, theme } = this.props;
const updateTheme = newTheme => () => serverStateManagement.updateTheme(newTheme, () => setTheme(newTheme));
return (
<li className="hoverable" style={{ color: "#565b68" }}>
<MenuItem style={{ color: "#565b68" }} description={Descriptions.theme}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since I'm looking at the theme option, have you tested your changes against "Dark Mode"? I don't believe the theme really changes anything about the menu, but might want to validate nothing screwy happens.

@@ -219,6 +219,8 @@ class ReactDataViewer extends React.Component {
return (
<GridEventHandler propagateState={this.propagateState} gridState={this.state}>
<DtaleHotkeys propagateState={this.propagateState} {...this.state} />
<MenuTooltip />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it work to keep the MenuTooltip within DataViewerMenu or does it need to be at the outer most level? Just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does work in DataViewerMenu, nice! That makes a lot more sense, so I'll change that. I wasn't sure if it needed to be outside the container with the overflow: scroll

@@ -23,6 +23,7 @@ import { ColumnMenu } from "./column/ColumnMenu";
import { exports as gu } from "./gridUtils";
import Descriptions from "./menu-descriptions.json";
import { DataViewerMenu } from "./menu/DataViewerMenu";
import { MenuTooltip } from "./menu/MenuTooltip";
Copy link
Collaborator

Choose a reason for hiding this comment

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

In order to fix horizontal scroll issue when the menu is pinned please add the following to this file:

  • Line 243: width={width - (this.props.menuPinned ? 198 : 3)}
  • Line 284: menuPinned: PropTypes.bool,
  • Line 288: ({ dataId, iframe, theme, settings, menuPinned }) => ({ dataId, iframe, theme, settings, menuPinned }),

@codecov-io
Copy link

Codecov Report

Merging #441 (33bd303) into master (654cbe7) will increase coverage by 1.03%.
The diff coverage is 81.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
+ Coverage   91.26%   92.30%   +1.03%     
==========================================
  Files         216      222       +6     
  Lines       10978    13265    +2287     
  Branches     1208     1215       +7     
==========================================
+ Hits        10019    12244    +2225     
- Misses        576      633      +57     
- Partials      383      388       +5     
Flag Coverage Δ
javascript 87.98% <81.35%> (-0.08%) ⬇️
python 96.32% <ø> (+0.64%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
static/dtale/GridEventHandler.jsx 90.27% <ø> (ø)
static/dtale/menu/DescribeOption.jsx 100.00% <ø> (ø)
static/dtale/menu/DuplicatesOption.jsx 100.00% <ø> (ø)
static/dtale/menu/HeatMapOption.jsx 100.00% <ø> (ø)
static/dtale/menu/InstancesOption.jsx 100.00% <ø> (ø)
static/dtale/menu/LowVarianceOption.jsx 100.00% <ø> (ø)
static/dtale/menu/MergeOption.jsx 100.00% <ø> (ø)
static/dtale/menu/NetworkOption.jsx 100.00% <ø> (ø)
static/dtale/menu/RangeHighlightOption.jsx 100.00% <ø> (ø)
static/dtale/menu/ThemeOption.jsx 100.00% <ø> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 654cbe7...33bd303. Read the comment docs.

@aschonfeld aschonfeld merged commit 5c82811 into man-group:master Feb 25, 2021
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.

3 participants