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

fix(zoombar): cache zoombar data and zoombar default domain #855

Merged
merged 14 commits into from
Nov 6, 2020
Merged

fix(zoombar): cache zoombar data and zoombar default domain #855

merged 14 commits into from
Nov 6, 2020

Conversation

scottdickerson
Copy link
Contributor

@scottdickerson scottdickerson commented Oct 30, 2020 •
edited
Loading

Updates

  • list
    new demo data, new high-scale testcase with 2000 data points in zoom bar
  • out
  • updates
    zoom-bar.ts, pass the calculated zoom bar data to the domain calculator on render
    zoom.ts, add the custom zoomBarData to the model if it exists and pull it from there, remove the sanitization logic as that will be done by @theiliad and that was causing the slowdown at render time

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)

@scottdickerson scottdickerson requested review from cal-smith and removed request for a team October 30, 2020 20:36
theiliad
theiliad reviewed Oct 30, 2020
View reviewed changes
Copy link
Member

@theiliad theiliad left a comment

Choose a reason for hiding this comment

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

Looks ok mostly, just a couple of concerns:

  1. What are the demo data additions? Do we need those or are you testing using those?
  2. How much have you tested this? Is there a chance that these changes will break some other demos or functionalities?

@scottdickerson
Copy link
Contributor Author

The demo data additions are just for testing

@scottdickerson
Copy link
Contributor Author

I tested all of the existing demos and they’re looking fine for zoombar

@hlyang397
Copy link
Contributor

@scottdickerson How can we update zoom bar data if it's necessary ?

@scottdickerson
Copy link
Contributor Author

@hlyang397 thanks I added a new method to model called setZoomBarData to address that, please take another look. @theiliad I think this fix is ready to merge when you have a chance to review it.

theiliad
theiliad requested changes Nov 2, 2020
View reviewed changes
Copy link
Member

@theiliad theiliad left a comment

Choose a reason for hiding this comment

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

Could you please delete this demo set entirely as well
image

packages/core/stories/utils.ts Show resolved Hide resolved
packages/core/stories/utils.ts Outdated Show resolved Hide resolved
theiliad
theiliad requested changes Nov 2, 2020
View reviewed changes
packages/core/stories/utils.ts Outdated Show resolved Hide resolved
scottdickerson and others added 2 commits November 2, 2020 12:39
@scottdickerson @theiliad
Update packages/core/stories/utils.ts
74b00a9 
Co-authored-by: Eliad Moosavi <theiliad@users.noreply.github.com>
@scottdickerson
fix(zoombar): still need to sanitize
9984f96
@theiliad
Copy link
Member

theiliad commented Nov 3, 2020 •
edited
Loading

sanitization logic will be available via #856

@scottdickerson
Copy link
Contributor Author

ok I've merged your stuff in now @theiliad and I'm ready for a merge

theiliad
theiliad requested changes Nov 4, 2020
View reviewed changes
packages/core/src/model.ts Outdated Show resolved Hide resolved
theiliad
theiliad requested changes Nov 4, 2020
View reviewed changes
packages/core/src/components/axes/zoom-bar.ts Show resolved Hide resolved
packages/core/src/model.ts Outdated Show resolved Hide resolved
@scottdickerson
Copy link
Contributor Author

I don't think I can make those suggested changes @theiliad but let me know if I'm incorrect, I did remove the DEV stories as part of my latest push

theiliad
theiliad previously approved these changes Nov 5, 2020
View reviewed changes
@sophiiae
Copy link
Contributor

sophiiae commented Nov 5, 2020

Screen Shot 2020-11-05 at 2 31 05 PM

seems something is broken here

natashadecoste
natashadecoste suggested changes Nov 5, 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.

you get a blank in the Angular storybook too in addition to @sophiiae comment on the react

@natashadecoste
Copy link
Contributor

@scottdickerson
it seems on the initial load there isnt any data but the model still tries to convert it to tabular data format.
the model tries to use the sanitize method to change the data but i think data isnt empty [] its actually undefined.

@scottdickerson
Copy link
Contributor Author

thanks @natashadecoste @sophiiae I've fixed, I'm just going to hide this story from the downstream examples for now (only show in Core) since I don't have access to the storyUtils to generate the data, it's ready for re-review

@theiliad theiliad merged commit 27e1725 into carbon-design-system:master Nov 6, 2020
@moczolaszlo moczolaszlo mentioned this pull request Nov 9, 2020
Closed
8 tasks
theiliad added a commit to theiliad/carbon-charts that referenced this pull request May 17, 2021
@scottdickerson @theiliad
Merge pull request carbon-design-system#855 from scottdickerson/fix-z…
36d801b 
…oombar-scale

* fix(zoombar): cache zoombar data and zoombar default domain

* fix(zoombar): cache custom zoom bar data in the model

* fix(zoombar): cache custom zoom bar data in the model

* Update packages/core/stories/utils.ts

Co-authored-by: Eliad Moosavi <theiliad@users.noreply.github.com>

* fix(zoombar): still need to sanitize

* test(dev): remove dev stories

* refactor(model-zoom): move methods zoom bar specific to model-zoom

* refactor(model-zoom): rename to model-cartesian-charts

* fix(lint): fix lint issue

* fix(stories): remove from non core stories

* fix(stories): remove from non core stories

Co-authored-by: Eliad Moosavi <theiliad@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natashadecoste natashadecoste natashadecoste approved these changes

@theiliad theiliad theiliad approved these changes

@cal-smith cal-smith Awaiting requested review from cal-smith

Assignees
No one assigned
Labels
type: enhancement 💡
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@scottdickerson @hlyang397 @theiliad @sophiiae @natashadecoste