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

Zoom bar #649

Merged
merged 162 commits into from
Jul 30, 2020
Merged

Zoom bar #649

merged 162 commits into from
Jul 30, 2020

Conversation

hlyang397
Copy link
Contributor

@hlyang397 hlyang397 commented Jun 2, 2020

Updates

For #380

  • add new ZoomBar component

#698

Demo screenshot or recording

image


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)

@hlyang397 hlyang397 requested review from natashadecoste, theiliad and a team as code owners June 2, 2020 09:28
@theiliad theiliad marked this pull request as draft June 4, 2020 14:21
@theiliad
Copy link
Member

theiliad commented Jun 4, 2020

converting this PR into a draft

@theiliad theiliad changed the title [In progress] Zoom bar Zoom bar Jun 4, 2020
@hlyang397 hlyang397 force-pushed the zoom-bar branch 2 times, most recently from d4ed056 to cd96b2b Compare June 10, 2020 08:08
@theiliad
Copy link
Member

Looking great :hlyang397 💯
One thing I noticed is that the zooming doesn't happen continuously (as you're moving the handles), that is probably a key aspect of the experience to have the graph update in real-time.

@hlyang397 hlyang397 force-pushed the zoom-bar branch 2 times, most recently from 6774132 to 3863d79 Compare June 13, 2020 07:32
@hlyang397
Copy link
Contributor Author

Looking great :hlyang397 💯
One thing I noticed is that the zooming doesn't happen continuously (as you're moving the handles), that is probably a key aspect of the experience to have the graph update in real-time.

@theiliad Thanks for your review.
We have many new commits in past few days, all comments are welcome, thanks.

@theiliad
Copy link
Member

theiliad commented Jun 16, 2020

Hi, looks great! 💯

Couple of pieces of feedback:

  1. we should enable the feature in more charts just to validate the functionality and make sure that it works everywhere

  2. I'm seeing a bug sometimes where the axes don't resize properly
    image

  3. as soon as I move the handles on both ends, they resize and become taller, they should probably stay the same height
    image

  4. Not sure if this is a bug but when I go any further to the left from this point, the graph takes no changes
    image

  5. The x-axis labels seem to be positioned incorrectly in the initial render, and once you interact with the chart they move back into their place
    image

  6. Looks like the handle is off by a few pixels
    image

  7. After the initial render if you try to pan, the graph would zoom in, axis would update and handles would slightly move
    image

@theiliad
Copy link
Member

question, do we have a way of setting the range of the zoomed area through options? e.g. if you were rendering a graph with many points and wanted it initially to zoom into an area and only show 10-20 points?

@theiliad
Copy link
Member

theiliad commented Jun 22, 2020

TODOs that I can see so far:

  • click&drag behavior on the graph should trigger panning not zooming

  • shift-clicking should result in zooming

  • handle toolbars aren't in yet

  • we need the non-highlighted areas in the zoom-bar to be darker in dark themes and lighter in light themes (I'd just draw 2 rectangles over both sides)

  • addSpaceOnEdges is doing nothing now when the zoom bar shows up

  • chart renders incorrectly initially and once you interact with the zoom bar it re-renders correctly
    image

  • tooltips are no longer working on the graphs

  • handles have the wrong height
    image

  • axis sometimes renders incorrectly
    image

@theiliad
Copy link
Member

@scottdickerson

@hlyang397
Copy link
Contributor Author

hlyang397 commented Jun 23, 2020

@theiliad,

Thanks for your review comment.

click&drag behavior on the graph should trigger panning not zooming
I think zooming makes more sense than panning when user selects an area.

Are you talking about the dragging event on the graph ? We didn't implement it yet.

shift-clicking should result in zooming
handle toolbars aren't in yet

We will include these features in next phase.

handles have the wrong height

The handle height is overwritten by d3.js brush library. I would put this as low priority before I can find the solution.

For rest problems, I have created issues in our tool.
Thanks.

@hlyang397 hlyang397 force-pushed the zoom-bar branch 2 times, most recently from 12f4fc4 to a37a6c4 Compare June 29, 2020 07:15
@hlyang397
Copy link
Contributor Author

@theiliad,
Please help to review the latest changes.

There are some issues still in progress, and may need more discussion.

  1. We think click & drag on the graph should trigger zooming not panning.
  2. Space on edges makes sense in some charts (ex: bar, bubble), but looks weird in the others(ex: line).
    With zoombar enabled, we may need to handle them accordingly.
  3. Tooltips are covered by the chart brush. Waiting for further investigation.
  4. Handle toolbar and shift-clicking zooming will be included in next phase (PR).

All comments are welcome, thanks.

@theiliad
Copy link
Member

@theiliad,
Please help to review the latest changes.

There are some issues still in progress, and may need more discussion.

  1. We think click & drag on the graph should trigger zooming not panning.
    @designertyler
  1. Space on edges makes sense in some charts (ex: bar, bubble), but looks weird in the others(ex: line).
    With zoombar enabled, we may need to handle them accordingly.
    we can't override what the user defines in options, if they add space on the edges, we need to show it.
  1. Tooltips are covered by the chart brush. Waiting for further investigation.
    image

would something like:

react.overlay:hover {
	pointer-events: none;
}

work?

we've gotta find a way to show tooltips on hover

  1. Handle toolbar and shift-clicking zooming will be included in next phase (PR).
    👍

All comments are welcome, thanks.

@hlyang397
Copy link
Contributor Author

@theiliad ,
Thanks for your comments.
2. That makes sense to me. I will try to make the addSpaceOnEdges option working with zoombar.
3. Setting pointer-events: none; breaks the brush. I fix that by moving brush component layer to back in this commit.
60990b9

@theiliad
Copy link
Member

@theiliad ,
Thanks for your comments.
2. That makes sense to me. I will try to make the addSpaceOnEdges option working with zoombar.
3. Setting pointer-events: none; breaks the brush. I fix that by moving brush component layer to back in this commit.
60990b9

So the singular tooltip works now but not the multiline tooltip. I think we might have to get rid of the brush overlay and release events such as grid-backdrop-drag etc... and show the zoom area by catching that

@hlyang397
Copy link
Contributor Author

hlyang397 commented Jun 30, 2020

@theiliad ,

So the singular tooltip works now but not the multiline tooltip. I think we might have to get rid of the brush overlay and release events such as grid-backdrop-drag etc... and show the zoom area by catching that

Could you give me an example of multiline tooltip? Thanks.

@theiliad
Copy link
Member

theiliad commented Jun 30, 2020

Could you give me an example of multiline tooltip? Thanks.

image

https://carbon-design-system.github.io/carbon-charts/?path=/story/line--line-time-series

@natashadecoste
Copy link
Contributor

right now the rect.zoom-bg is going outside the chart container
image

natashadecoste
natashadecoste previously approved these changes Jul 30, 2020
sophiiae
sophiiae previously approved these changes Jul 30, 2020
@theiliad theiliad dismissed stale reviews from sophiiae and natashadecoste via cf312ab July 30, 2020 19:12
@theiliad theiliad merged commit 4502861 into carbon-design-system:master Jul 30, 2020
theiliad added a commit to theiliad/carbon-charts that referenced this pull request May 17, 2021
* feat: initial zoom bar implementation from PR 511

* fix: update ZoomBar to match new data format

- fix trailing-comma
- update scales-cartesian.getValueFromScale()
- update line time-series 15 seconds demo data

* feat: use d3-brush to implement selection

* fix: add left margin to align to chart

- using .scss to set fill and stroke color

* feat: set brush selection to zoomDomain in Model

- use flag to avoid recursive update event

* feat: create ZoomBarOptions and default configuration

- default to disable zoombar

* feat: create zoombar story in storybook

* fix: set tslint trailing-comma to never

* fix: allow zoomDomain to update for every brush event

* feat: provide ZoomBar options for brush event callback

- selected x range and domain as callback parameters

* feat: apply clipPath to line chart - v1

* feat: apply cover to axis chart

* refactor: clean code

* feat: set axes margins as zoom bar left margin

* refactor: add todo

* fix: double the minTickSize for datetime ticks

* feat: add chart brush

* refactor: add brush in axis chart component

* refactor: move brush functions to ZoomBar class function

* fix: handel situation of zoom bar selection[0] equals selection[1]

* feat: ZoomBar could update brush based on zoomDomain

- fix some brush handle UI bugs

* fix: handle empty selection

* fix: set to default full range when same selection of brush

* fix: fix bug when selection === null

* feat: add ZoomBarOptions.initZoomDomain

- update storybook
- add init() and destroy() in ZoomBar

* feat: create Zoombar storybook demo group

* fix: apply cover to stacked bar

* feat: create ZoomBar event and handler

- use Events.ZoomBar.UPDATE to handle axesMargins update

* fix: fix merge error

- move ZoomBarOptions from BaseChartOptions to AxisChartOptions

* feat: update color for non-highlighted areas

- remove zoombar graph line
- add zoombar unselected graph area with clip
- add zoombar baseline
- clear d3.brush selection style

* fix: avoid extra brush handle update

* fix: fix the brush handle style

* refactor: remove unnecessary svg.brush-container

* fix: move brush layer to back to allow tooltips in graph

* feat: add stacked-area chart with zoom bar in storybook

* refactor: delete unused axis-zoom service

* feat: allow addSpaceOnEdges to work with zoom bar

- extends domain in zoom bar and brush

* refactor: code refactoring

- reduce code differences from master branch

* fix: display multiline tooltip with zoom bar

- use ruler backdrop as brush area
- svg.chart-grid-backdrop

* feat: create ZoomBarOptions and default configuration

- default to disable zoombar

* fix: chart brush with correct range

* fix: remove graph out of zoom domain

* refactor: code refactoring

- create getZoomBarData(), getDefaultZoomBarDomain() in model
- remove unused code

* fix: set min selection difference threshold

* refactor: code refactoring

- zoom-bar.scss
- remove unused scss settings

* fix: avoid extra/duplicate external callback

* fix: remove ZoomBarOptions in BaseChartOptions

* refactor: change initZoomDomain to initialZoomDomain

- remove unnecessary undefined setting

* refactor: use Tools.getProperty to load zoomBarOptions

* refactor: update code format

* refactor: use Tools.getProperty to get initialZoomDomain

* refactor: Change Cover to ChartClip

* refactor: Change Brush to ChartBrush

* refactor: set model.set() function default config

* fix: remove unnecessary selector

* refactor: create reusable getMainXScaleType()

* refactor: make sure zoom bar only shows with supported options

- zoomBar is available when
  -- zoomBar is enabled
  -- main X axis position is bottom
  -- main X axis scale type is time

* refactor: set clip-path url in containerSVG

* refactor: dispatch zoombar selection events instead of callback

* feat: update chart brush selection storke to dash

* feat: show tooltip when mouseover zoombar handle

* fix: avoid tick rotation flip during zoom domain changing

- always rotate ticks during zoom domain changing

* fix: move chart brush selection above all graphs

* fix: use another svg to display front selection

* fix: set cursor for front selection

* fix: use unique chartClipId for each chart

* fix: keep zoom bar handle inside zoom bar range

* fix: use DOMUtils.appendOrSelect in case the element does not exist yet

* feat: allow user to set zoom bar data

* fix: definedZoomBarData needs at least two elements

* fix: axis needs to set opacity if not loading

* fix: format zoom bar handle tooltip

* fix: don't set brush handle tooltip if domain is undefined

* fix: filter out invalid domain value to avoid exception

* feat: initial zoom bar implementation from PR 511

* fix: update ZoomBar to match new data format

- fix trailing-comma
- update scales-cartesian.getValueFromScale()
- update line time-series 15 seconds demo data

* feat: use d3-brush to implement selection

* fix: add left margin to align to chart

- using .scss to set fill and stroke color

* feat: set brush selection to zoomDomain in Model

- use flag to avoid recursive update event

* feat: create ZoomBarOptions and default configuration

- default to disable zoombar

* feat: create zoombar story in storybook

* fix: set tslint trailing-comma to never

* fix: allow zoomDomain to update for every brush event

* feat: provide ZoomBar options for brush event callback

- selected x range and domain as callback parameters

* feat: apply clipPath to line chart - v1

* feat: apply cover to axis chart

* refactor: clean code

* feat: set axes margins as zoom bar left margin

* refactor: add todo

* fix: double the minTickSize for datetime ticks

* feat: add chart brush

* refactor: add brush in axis chart component

* refactor: move brush functions to ZoomBar class function

* fix: handel situation of zoom bar selection[0] equals selection[1]

* feat: ZoomBar could update brush based on zoomDomain

- fix some brush handle UI bugs

* fix: handle empty selection

* fix: set to default full range when same selection of brush

* fix: fix bug when selection === null

* feat: add ZoomBarOptions.initZoomDomain

- update storybook
- add init() and destroy() in ZoomBar

* feat: create Zoombar storybook demo group

* fix: apply cover to stacked bar

* feat: create ZoomBar event and handler

- use Events.ZoomBar.UPDATE to handle axesMargins update

* fix: fix merge error

- move ZoomBarOptions from BaseChartOptions to AxisChartOptions

* feat: update color for non-highlighted areas

- remove zoombar graph line
- add zoombar unselected graph area with clip
- add zoombar baseline
- clear d3.brush selection style

* fix: avoid extra brush handle update

* fix: fix the brush handle style

* refactor: remove unnecessary svg.brush-container

* fix: move brush layer to back to allow tooltips in graph

* feat: add stacked-area chart with zoom bar in storybook

* refactor: delete unused axis-zoom service

* feat: allow addSpaceOnEdges to work with zoom bar

- extends domain in zoom bar and brush

* refactor: code refactoring

- reduce code differences from master branch

* fix: display multiline tooltip with zoom bar

- use ruler backdrop as brush area
- svg.chart-grid-backdrop

* feat: create ZoomBarOptions and default configuration

- default to disable zoombar

* fix: chart brush with correct range

* fix: remove graph out of zoom domain

* refactor: code refactoring

- create getZoomBarData(), getDefaultZoomBarDomain() in model
- remove unused code

* fix: set min selection difference threshold

* refactor: code refactoring

- zoom-bar.scss
- remove unused scss settings

* fix: avoid extra/duplicate external callback

* fix: remove ZoomBarOptions in BaseChartOptions

* refactor: change initZoomDomain to initialZoomDomain

- remove unnecessary undefined setting

* refactor: use Tools.getProperty to load zoomBarOptions

* refactor: update code format

* refactor: use Tools.getProperty to get initialZoomDomain

* refactor: Change Cover to ChartClip

* refactor: Change Brush to ChartBrush

* refactor: set model.set() function default config

* fix: remove unnecessary selector

* refactor: create reusable getMainXScaleType()

* refactor: make sure zoom bar only shows with supported options

- zoomBar is available when
  -- zoomBar is enabled
  -- main X axis position is bottom
  -- main X axis scale type is time

* refactor: set clip-path url in containerSVG

* refactor: dispatch zoombar selection events instead of callback

* feat: update chart brush selection storke to dash

* feat: show tooltip when mouseover zoombar handle

* fix: avoid tick rotation flip during zoom domain changing

- always rotate ticks during zoom domain changing

* fix: move chart brush selection above all graphs

* fix: use another svg to display front selection

* fix: set cursor for front selection

* fix: use unique chartClipId for each chart

* fix: keep zoom bar handle inside zoom bar range

* fix: use DOMUtils.appendOrSelect in case the element does not exist yet

* feat: allow user to set zoom bar data

* fix: definedZoomBarData needs at least two elements

* fix: axis needs to set opacity if not loading

* fix: format zoom bar handle tooltip

* fix: don't set brush handle tooltip if domain is undefined

* fix: filter out invalid domain value to avoid exception

* fix: never out of zoom domain if only one bar in chart

* fix: don't set zoomDomain to invalid value

* refactor code

* format

* feat: add zoom bar skeleton and demo

* update zoom bar option APIs

* bugfix for tooltips

* add ChartClip to charts only when zoombar is enabled

* format

* update interfaces

* fix zoombar background width issue

Co-authored-by: Jennifer Chao <[email protected]>
Co-authored-by: Jennifer Chao <[email protected]>
Co-authored-by: Eliad Moosavi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants