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

Jump to bottom

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

Closed
wants to merge 527 commits into from
Closed

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

wants to merge 527 commits into from

Conversation

JennChao
Copy link
Contributor

Updates

Demo screenshot or recording

圖片

Review checklist (for reviewers only)

  • Demos all features
  • Documented/annotated
  • Matches UI/UX specs
  • Meets the code style guide
  • Accessible
  • Mobile first (responsive)
  • RTL support (bidirectional text)
  • Performant (limited bloat)

@JennChao
Copy link
Contributor Author

@theiliad
Here comes the draft PR for tool bar.
Please take a look. Thanks!

@JennChao JennChao changed the title Tool bar above zoom bar Tool bar above zoom bar and shift-clicking on chart Jul 22, 2020
@theiliad theiliad requested a review from designertyler July 22, 2020 13:33
hlyang397 and others added 27 commits July 23, 2020 10:25
@hlyang397
feat: update chart brush selection storke to dash
8aa950b
@hlyang397
feat: show tooltip when mouseover zoombar handle
ad2f115
@hlyang397
fix: avoid tick rotation flip during zoom domain changing
e903a6a 
- always rotate ticks during zoom domain changing
@hlyang397
fix: move chart brush selection above all graphs
3b7413e
@hlyang397
fix: use another svg to display front selection
d974b60
@hlyang397
fix: set cursor for front selection
ac6ed57
@hlyang397
fix: use unique chartClipId for each chart
fa1ac37
@hlyang397
fix: keep zoom bar handle inside zoom bar range
80324a2
@hlyang397
fix: use DOMUtils.appendOrSelect in case the element does not exist yet
3ccef0d
@hlyang397
feat: allow user to set zoom bar data
68993ec
@hlyang397
fix: definedZoomBarData needs at least two elements
1b0b962
@hlyang397
fix: axis needs to set opacity if not loading
93cce4a
@hlyang397
fix: format zoom bar handle tooltip
eebb780
@hlyang397
fix: don't set brush handle tooltip if domain is undefined
2e76f0a
@JennChao @JenniferChao-tc
Merge branch 'zoom-bar' of https://github.com/hlyang397/carbon-charts …
c04d0db 
…into tool-bar
@hlyang397
feat: initial zoom bar implementation from PR 511
563f680
@hlyang397
fix: update ZoomBar to match new data format
1ff4343 
- fix trailing-comma
- update scales-cartesian.getValueFromScale()
- update line time-series 15 seconds demo data
@hlyang397
feat: use d3-brush to implement selection
6845aff
@hlyang397
fix: add left margin to align to chart
5efec0d 
- using .scss to set fill and stroke color
@hlyang397
feat: set brush selection to zoomDomain in Model
b8d7d38 
- use flag to avoid recursive update event
@hlyang397
feat: create ZoomBarOptions and default configuration
2208434 
- default to disable zoombar
@hlyang397
feat: create zoombar story in storybook
aed57ea
@hlyang397
fix: set tslint trailing-comma to never
fc1fccc
@hlyang397
fix: allow zoomDomain to update for every brush event
c2c700e
@hlyang397
feat: provide ZoomBar options for brush event callback
d5d512e 
- selected x range and domain as callback parameters
@JennChao @hlyang397
feat: apply clipPath to line chart - v1
48a313d
@JenniferChao-tc @hlyang397
feat: apply cover to axis chart
fd250d5
@hlyang397
refactor: remove duplicate zoom service binding
1e6ff2a
@hlyang397
refactor: update per comment
ba21aeb 
- remove this.isZoomBarEnabled, this.iconPadding
- append random number to resetZoomMenuItemId
- remove unnecessary this.init()
@hlyang397
refactor: remove setOverflowMenuIconHover()
ad2c4fb
@hlyang397
Copy link
Contributor

hlyang397 commented Aug 11, 2020 •
edited
Loading

@JennChao and I have some discussion, we prefer to follow Carbon component naming convention.
So we will have the overflow icon as one of toolbar button and after clicking the icon, an overflow menu will popup, and overflow menu could contain multiple overflow menu item .
@theiliad Please let me know if you have any question, thanks.
Updated in a956942.

@hlyang397
Copy link
Contributor

@theiliad I have updated the code based on your comments.
Please help to review again and let me know if any other comment, thanks.

natashadecoste
natashadecoste suggested changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@natashadecoste natashadecoste left a comment

Choose a reason for hiding this comment

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

we allow charts to not have titles (they might be within a dashboard card that already has a title, or whatever the case).
if we dont have a title, the toolbar is left aligned but i feel like the actions should stay to the right (as overflow menus usually do) - i would think we have a spec for this

the open overflow menu opens on the right when the icons are on the left
image

@hlyang397
Merge branch 'master' into tool-bar
f1b5747 
# Conflicts:
#	packages/core/src/components/axes/zoom-bar.ts
#	packages/core/src/configuration.ts
#	packages/core/src/interfaces/components.ts
@hlyang397
fix: keep title component if toolbar is enabled for alignment
fa0f2fe
@hlyang397
Copy link
Contributor

hlyang397 commented Aug 12, 2020 •
edited
Loading

we allow charts to not have titles (they might be within a dashboard card that already has a title, or whatever the case).
if we dont have a title, the toolbar is left aligned but i feel like the actions should stay to the right (as overflow menus usually do) - i would think we have a spec for this

@natashadecoste
Thanks for catching this.
You are right, the toolbar should be right aligned.
I have pushed a fix fa0f2fe for the no title chart with toolbar enabled.
Thanks.

image

sophiiae
sophiiae previously approved these changes Aug 13, 2020
View reviewed changes
@hlyang397
Copy link
Contributor

@theiliad @natashadecoste @sophiiae
Any update for this PR?

@theiliad
Copy link
Member

@JennChao could you please resolve the conflicts?

@hlyang397 hlyang397 mentioned this pull request Sep 8, 2020
Merged
8 tasks
@hlyang397
Copy link
Contributor

Replaced by #766 #773 #791
Closed this PR.

@hlyang397
Copy link
Contributor

@theiliad
It looks like I don't have permission to close this PR, could you help to close this one ? Thanks.

@theiliad theiliad closed this Sep 8, 2020
@theiliad
Copy link
Member

theiliad commented Sep 8, 2020

Closed in favor of #791

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hlyang397 hlyang397 hlyang397 left review comments

@theiliad theiliad theiliad requested changes

@natashadecoste natashadecoste natashadecoste requested changes

@sophiiae sophiiae sophiiae left review comments

@designertyler designertyler Awaiting requested review from designertyler

@zvonimirfras zvonimirfras Awaiting requested review from zvonimirfras

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JennChao @theiliad @hlyang397 @natashadecoste @sophiiae @JenniferChao-tc