Toolbar #791

hlyang397 · 2020-09-08T06:39:32Z

Updates

Based on PR Tool bar above zoom bar and shift-clicking on chart #717
Toolbar
- zoom-in, zoom-out button
  - disabled state
- overflow menu button
  - reset zoom
  - disabled state
Toolbar demo group

Demo screenshot or recording

Review checklist (for reviewers only)

- toolbar configuration - zoom in/out icon - overflow menu - enable toolbar in zoom bar storybook

- remove toolbar options in zoom bar demo group

- simplify css - refactor toolbar button generation code

- allow button disabled state - check min/max zoom domain for zoom in/out button

theiliad · 2020-09-17T15:59:07Z

packages/core/src/configuration.ts

+	toolbar: {
+		enabled: false,
+		overflowMenuItems: {
+			resetZoom: {
+				enabled: false,
+				text: "Reset zoom"
+			} as ToolbarOverflowMenuItemOptions
+		} as ToolbarOverflowMenuItems
+	} as ToolbarOptions


So what's been discussed with Josh Kimmel and the rest of the dataviz guild would work a bit differently than specified here.

The user would need a way of defining how many icons to show and how many to move into the overflow.

This means that each item should have a name & icon, and the items can no longer be called overflow items but more so just controls in general.

That way the user can define a max number (e.g. 2) which means that the zoom in & zoom out buttons would show up as icons and everything else would go inside the overflow menu.

Additionally, the user needs to have a way of changing the order (e.g. define the order as zoom in, zoom out, reset zoom, make fullscreen, print and choose the limit as 3 which'll mean they'll only see zoom in, zoom out & reset zoom as icons)

Not sure if this is the best route to go but it's what design is asking for. I'll tag @jeanservaas here for feedback

jeanservaas · 2020-09-17T16:08:22Z

Yes I think this is what we agreed upon in the call... The user can basically arrange the controls they way they want, but we have some best practice suggestions

The things we strongly encourage:

if you have two or three controls, they will appear as icons in the upper right corner, not in an overflow.
If you have four or more controls, 2+ controls can appear in the overflow menu

Things we definitely want to avoid:

— Having only 1 item in an overflow menu
— Splitting up dependent controls like zoom up/zoom down (i.e. don't ever have a situation where one is in an overflow menu and one is not)

# Conflicts: # packages/core/src/configuration.ts # packages/core/src/interfaces/components.ts # packages/core/src/services/zoom.ts

- create ToolbarControlTypes enum - change configuration and interfaces - decide how many controls are in button and in overflow menu - change svg icon from 16x16 to 32x32 viewBox

hlyang397 · 2020-09-22T05:18:14Z

@theiliad @jeanservaas
I have updated this PR based on the new spec you mentioned.
Please help to review it, thanks.

theiliad · 2020-09-23T15:37:11Z

@theiliad @jeanservaas
I have updated this PR based on the new spec you mentioned.
Please help to review it, thanks.

Hi, it seems like deploy previews aren't working and Travis is failing. Might be some errors that we'd need to fix in the code.

You should be seeing some errors when running or building storybook locally.

hlyang397 · 2020-09-24T02:45:06Z

Hi, it seems like deploy previews aren't working and Travis is failing. Might be some errors that we'd need to fix in the code.

You should be seeing some errors when running or building storybook locally.

@theiliad
Thanks for catching this.
The local storybook is good, but I import src folder in storybook demo folder, and that won't work in dist/.
I have fixed this in latest commit.
Please let me know if you find any other issue.

theiliad

💯
We need the controls in the toolbar to be keyboard accessible (user should be able to tab through), and possibly proper aria labeling. Please see the legend items as an example in the demos

- make control focusable - specify roles in control

hlyang397 · 2020-09-30T04:23:47Z

💯
We need the controls in the toolbar to be keyboard accessible (user should be able to tab through), and possibly proper aria labeling. Please see the legend items as an example in the demos

@theiliad Can Carbon Charts includes Carbon components as library ?
Toolbar controls (button, overflow menu) are highly related to carbon design system, and have dependency on carbon components. It makes no sense to reinvent the wheel.

jeanservaas

LGTM! Thanks for the update

# Conflicts: # packages/core/src/configuration.ts # packages/core/src/services/zoom.ts

packages/core/src/services/zoom.ts

packages/core/src/interfaces/components.ts

packages/core/demo/data/toolbar.ts

theiliad · 2020-10-13T15:38:36Z

packages/core/src/components/axes/toolbar.ts

+	}
+
+	// show/hide overflow menu
+	showOverflowMenu(show: boolean) {


so in the comments it says show/hide overflow menu however the method name is showOverflowMenu 😄

@theiliad
Yes, we use the boolean parameter to decide if we should show or hide the overflow menu.
I am not pretty sure what's your preference.
Do you like to have two separate functions (showOverflowMenu and hideOverflowMenu) ?
Or any other preferred function name ?

My concern is the name of the function. Looking at the name showOverflowMenu I'd assume that calling it would show the overflow menu. Maybe we need to call it updateOverflowMenu

theiliad · 2020-10-13T15:39:59Z

packages/core/src/components/axes/toolbar.ts

+
+	toggleOverflowMenu() {
+		if (this.isOverflowMenuOpen()) {
+			// hide overflow menu


// hide overflow menu ...... => this.showOverflowMenu(false); 😄

Please refer to #791 (comment).

theiliad · 2020-10-13T15:46:30Z

packages/core/src/components/axes/toolbar.ts

+			switch (control.type) {
+				case ToolbarControlTypes.ZOOM_IN:
+					if (isZoomBarEnabled) {
+						controlConfig = this.getZoomInConfig();


seems like we can have 1 method called getControlConfigs(control: ToolbarControlTypes) which'll just give you the right config rather than implementing a switch and having multiple config methods

@theiliad
I don't get your point.
Even you have a getControlConfigs(control: ToolbarControlTypes) method, you still need a switch in the method to return respective config based on ToolbarControlTypes.
Could you provide a pseudo code or example ?

Hi, yes what I'm trying to avoid is a getZoomInConfig() call for a small object return. This'll result in too many methods in the future.

If we just have a getControlConfigs() that includes all the configs and returns the 1 we care about, then it's simpler

Updated in bcee77b.
Please let me know if this is not what you expected, thanks.

packages/core/src/components/axes/toolbar.ts

theiliad · 2020-10-13T17:16:19Z

packages/core/src/components/axes/toolbar.ts

+			.attr("opacity", 1);
+
+		// clean children first
+		container.html(null);


hmmmm... so the toolbar cleans everything everytime? Why not render more efficiently through d3?

Yes, we need to clean buttons if data is loading, or update toolbar controls if configuration is changed.
We could use d3, but I don't think it could help to reduce the code complexity because the buttons and overflow menu items are more related to configurations instead of simple data, and for each configuration we need to create complex html component instead of SVG.
Pleasee let me know if you still think we should change to use d3.

theiliad

added comments above

hlyang397 · 2020-10-14T11:54:17Z

added comments above

@theiliad I have updated PR based on your comments. Please help to review again, thanks.

theiliad · 2020-10-14T18:08:56Z

packages/core/src/components/axes/toolbar.ts

+			switch (control.type) {
+				case ToolbarControlTypes.ZOOM_IN:
+					if (isZoomBarEnabled) {
+						controlConfig = this.getZoomInConfig();


Hi, yes what I'm trying to avoid is a getZoomInConfig() call for a small object return. This'll result in too many methods in the future.

If we just have a getControlConfigs() that includes all the configs and returns the 1 we care about, then it's simpler

hlyang397 · 2020-10-23T02:47:11Z

@theiliad Any other comment on this PR?

theiliad · 2020-10-26T14:41:24Z

packages/core/demo/data/toolbar.ts

+		},
+		{
+			type: "Reset zoom",
+			text: "Reset zoom"


Maybe it'd better to just include a small tutorial story for the toolbar and describe this there. I wouldn't want the text in the demo code, that'll result in users copying that code over to their projects, and it'll show up in codesandbox

packages/core/demo/data/toolbar.ts

packages/core/src/components/axes/toolbar.ts

theiliad · 2020-10-26T15:04:38Z

packages/core/src/components/axes/toolbar.ts

+	}
+
+	// show/hide overflow menu
+	showOverflowMenu(show: boolean) {


My concern is the name of the function. Looking at the name showOverflowMenu I'd assume that calling it would show the overflow menu. Maybe we need to call it updateOverflowMenu

packages/core/src/components/axes/toolbar.ts

- titlePostfix -> titleSuffix - showOverflowMenu -> updateOverflowMenu - VERTICAL_ALIGNMENT_Y -> Y_OFFSET

hlyang397 · 2020-10-27T07:01:27Z

@theiliad Updated based on your comments.
Please help to review again, thanks.

natashadecoste

Nice
💯 for documention too

…lbar * feat: create toolbar components - toolbar configuration - zoom in/out icon - overflow menu - enable toolbar in zoom bar storybook * refactor: use constants in configuration * feat: create toolbar storybook demo group - remove toolbar options in zoom bar demo group * refactor: toolbar button - simplify css - refactor toolbar button generation code * feat: disable toolbar button if it won't work - allow button disabled state - check min/max zoom domain for zoom in/out button * fix: title should use parent node to get max title width * feat: provide minZoomRatio option * feat allow overflow menu item to be disabled * feat: combine toolbar icon and overflow menu item to controls - create ToolbarControlTypes enum - change configuration and interfaces - decide how many controls are in button and in overflow menu - change svg icon from 16x16 to 32x32 viewBox * fix: not import src/ in demo folder to avoid build failure * feat: support keyboard event for toolbar button - make control focusable - specify roles in control * refactor: fix scss style * refactor: change configuration name - maxIcons => numberOfIcons - controlsInOrder => controls * refactor: remove default ToolbarControls in configuration * feat: allow to use keyboard to control overflow menu * fix: align toolbar to title component * refactor: refactor statements and fix typos * refactor: change ToolbarControl.text field to optional * refactor: use getControlConfigByType() to reduce methods count * refactor: minor changes per comments - titlePostfix -> titleSuffix - showOverflowMenu -> updateOverflowMenu - VERTICAL_ALIGNMENT_Y -> Y_OFFSET * refactor: remove toolbar control text in demo * refactor: use d3.select to replace document.getElementById Co-authored-by: Fei Z <[email protected]>

hlyang397 added 7 commits September 3, 2020 13:55

feat: create toolbar components

ebc79e2

- toolbar configuration - zoom in/out icon - overflow menu - enable toolbar in zoom bar storybook

refactor: use constants in configuration

e611cbf

feat: create toolbar storybook demo group

4febc82

- remove toolbar options in zoom bar demo group

refactor: toolbar button

fc89ba2

- simplify css - refactor toolbar button generation code

feat: disable toolbar button if it won't work

3294ec2

- allow button disabled state - check min/max zoom domain for zoom in/out button

fix: title should use parent node to get max title width

ac8c1f7

feat: provide minZoomRatio option

a3ed332

hlyang397 requested review from natashadecoste, theiliad and a team as code owners September 8, 2020 06:39

hlyang397 requested review from Donisius and removed request for a team September 8, 2020 06:39

hlyang397 mentioned this pull request Sep 8, 2020

Tool bar above zoom bar and shift-clicking on chart #717

Closed

8 tasks

Fei Z and others added 3 commits September 10, 2020 15:46

Merge branch 'master' into create-toolbar

65ade54

Merge branch 'master' into create-toolbar

b5e355f

feat allow overflow menu item to be disabled

9014561

theiliad requested changes Sep 17, 2020

View reviewed changes

hlyang397 added 3 commits September 21, 2020 15:11

Merge branch 'master' into create-toolbar

4080fb6

# Conflicts: # packages/core/src/configuration.ts # packages/core/src/interfaces/components.ts # packages/core/src/services/zoom.ts

feat: combine toolbar icon and overflow menu item to controls

840c452

- create ToolbarControlTypes enum - change configuration and interfaces - decide how many controls are in button and in overflow menu - change svg icon from 16x16 to 32x32 viewBox

Merge branch 'master' into create-toolbar

71f66f1

hlyang397 requested a review from theiliad September 22, 2020 05:17

hlyang397 added 2 commits September 24, 2020 10:04

Merge branch 'master' into create-toolbar

1ca8e64

fix: not import src/ in demo folder to avoid build failure

fe574fd

theiliad requested changes Sep 25, 2020

View reviewed changes

feat: support keyboard event for toolbar button

081a301

- make control focusable - specify roles in control

jeanservaas previously approved these changes Oct 6, 2020

View reviewed changes

Merge branch 'master' into create-toolbar

4ca109d

# Conflicts: # packages/core/src/configuration.ts # packages/core/src/services/zoom.ts

hlyang397 dismissed jeanservaas’s stale review via 4ca109d October 12, 2020 03:24

hlyang397 requested a review from jeanservaas October 12, 2020 03:36

theiliad reviewed Oct 13, 2020

View reviewed changes

theiliad requested changes Oct 13, 2020

View reviewed changes

hlyang397 added 2 commits October 14, 2020 14:39

refactor: refactor statements and fix typos

4c136d2

refactor: change ToolbarControl.text field to optional

1c7e6ca

theiliad requested changes Oct 14, 2020

View reviewed changes

refactor: use getControlConfigByType() to reduce methods count

bcee77b

hlyang397 requested a review from theiliad October 15, 2020 04:36

theiliad requested changes Oct 26, 2020

View reviewed changes

theiliad added the type: enhancement 💡 label Oct 26, 2020

hlyang397 added 4 commits October 27, 2020 13:00

Merge branch 'master' into create-toolbar

d9ccac3

refactor: minor changes per comments

ffb41bc

- titlePostfix -> titleSuffix - showOverflowMenu -> updateOverflowMenu - VERTICAL_ALIGNMENT_Y -> Y_OFFSET

refactor: remove toolbar control text in demo

0f1889d

refactor: use d3.select to replace document.getElementById

70f7497

hlyang397 requested a review from theiliad October 27, 2020 07:01

theiliad approved these changes Oct 27, 2020

View reviewed changes

theiliad requested review from jeanservaas and removed request for jeanservaas October 27, 2020 17:12

natashadecoste approved these changes Oct 27, 2020

View reviewed changes

sophiiae approved these changes Oct 27, 2020

View reviewed changes

theiliad merged commit de64e26 into carbon-design-system:master Oct 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toolbar #791

Toolbar #791

hlyang397 commented Sep 8, 2020 •

edited

Loading

theiliad Sep 17, 2020

jeanservaas commented Sep 17, 2020

hlyang397 commented Sep 22, 2020

theiliad commented Sep 23, 2020

hlyang397 commented Sep 24, 2020

theiliad left a comment •

edited

Loading

hlyang397 commented Sep 30, 2020

jeanservaas left a comment

theiliad Oct 13, 2020

hlyang397 Oct 14, 2020

theiliad Oct 26, 2020

theiliad Oct 13, 2020

hlyang397 Oct 14, 2020

theiliad Oct 13, 2020

hlyang397 Oct 14, 2020

theiliad Oct 14, 2020

hlyang397 Oct 15, 2020

theiliad Oct 13, 2020

hlyang397 Oct 14, 2020

theiliad left a comment

hlyang397 commented Oct 14, 2020

theiliad Oct 14, 2020

hlyang397 commented Oct 23, 2020

theiliad Oct 26, 2020

theiliad Oct 26, 2020

hlyang397 commented Oct 27, 2020

natashadecoste left a comment

Toolbar #791

Toolbar #791

Conversation

hlyang397 commented Sep 8, 2020 • edited Loading

Updates

Demo screenshot or recording

Review checklist (for reviewers only)

Choose a reason for hiding this comment

jeanservaas commented Sep 17, 2020

hlyang397 commented Sep 22, 2020

theiliad commented Sep 23, 2020

hlyang397 commented Sep 24, 2020

theiliad left a comment • edited Loading

Choose a reason for hiding this comment

hlyang397 commented Sep 30, 2020

jeanservaas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theiliad left a comment

Choose a reason for hiding this comment

hlyang397 commented Oct 14, 2020

Choose a reason for hiding this comment

hlyang397 commented Oct 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlyang397 commented Oct 27, 2020

natashadecoste left a comment

Choose a reason for hiding this comment

hlyang397 commented Sep 8, 2020 •

edited

Loading

theiliad left a comment •

edited

Loading