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(svelte): fix BaseChart destroy behavior #800

Merged
merged 3 commits into from
Sep 14, 2020
Merged

fix(svelte): fix BaseChart destroy behavior #800

merged 3 commits into from
Sep 14, 2020

Conversation

metonym
Copy link
Contributor

@metonym metonym commented Sep 10, 2020 •
edited
Loading

Fixes #799

Updates

  • fix: rewrite component destroy method of BaseChart.svelte for usage with the svelte:component API
  • refactor: initialize ref, chart variables to null instead of undefined

TODO:

  • test with svelte:component
  • test with unmounting instance
  • test with swapping charts and updating data/options
  • test in Storybook

Demo screenshot or recording

Screen Shot 2020-09-11 at 6 33 41 AM

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)

@metonym metonym mentioned this pull request Sep 11, 2020
Closed
5 tasks
@metonym metonym changed the title [in progress] fix(svelte): rewrite BaseChart to reload chart instance [in progress] fix(svelte): fix BaseChart destroy behavior Sep 11, 2020
@metonym metonym marked this pull request as ready for review September 11, 2020 13:34
@metonym metonym requested review from natashadecoste, theiliad and a team as code owners September 11, 2020 13:34
@metonym metonym requested review from zvonimirfras and removed request for a team September 11, 2020 13:34
theiliad
theiliad approved these changes Sep 11, 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.

Makes sense, mostly relying on your testing and judgement on this Eric 💯

@metonym metonym changed the title [in progress] fix(svelte): fix BaseChart destroy behavior fix(svelte): fix BaseChart destroy behavior Sep 11, 2020
cal-smith
cal-smith approved these changes Sep 11, 2020
View reviewed changes
Copy link
Contributor

@cal-smith cal-smith left a comment

Choose a reason for hiding this comment

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

Makes sense to me 👍 given the context, I can totally see where the runtime would get confused

@theiliad theiliad merged commit 7de3fb9 into carbon-design-system:master Sep 14, 2020
@metonym metonym deleted the fix-svelte-base-chart branch September 14, 2020 15:07
JennChao added a commit to JennChao/carbon-charts that referenced this pull request Sep 15, 2020
@JennChao
Merge branch 'master' of https://github.com/JennChao/carbon-charts in…
34adc72 
…to ruler-option

* 'master' of https://github.com/JennChao/carbon-charts:
  v0.37.1
  Merge pull request carbon-design-system#800 from metonym/fix-svelte-base-chart
  v0.37.0
  Merge pull request carbon-design-system#780 from natashadecoste/fix-meter-max-val
  feat: enable or disable scatter dot on charts except scatter chart (carbon-design-system#769)
  feat: create options for tick rotation (carbon-design-system#770)
JennChao added a commit to JennChao/carbon-charts that referenced this pull request Sep 15, 2020
@JennChao
Merge branch 'master' of https://github.com/JennChao/carbon-charts in…
b00aad9 
…to tooltip-option

* 'master' of https://github.com/JennChao/carbon-charts:
  v0.37.1
  Merge pull request carbon-design-system#800 from metonym/fix-svelte-base-chart
  v0.37.0
  Merge pull request carbon-design-system#780 from natashadecoste/fix-meter-max-val
  feat: enable or disable scatter dot on charts except scatter chart (carbon-design-system#769)
  feat: create options for tick rotation (carbon-design-system#770)
  v0.36.4
  Merge pull request carbon-design-system#761 from JennChao/sparkline
  Merge pull request carbon-design-system#793 from hlyang397/update-initial-zoom-domain
  fix the defect of label tooltip not showing when using custom tooltip (carbon-design-system#787)
  v0.36.3
  Merge pull request carbon-design-system#785 from sophiiae/skeleton-with-data
JennChao added a commit to JennChao/carbon-charts that referenced this pull request Sep 15, 2020
@JennChao
Merge branch 'master' of https://github.com/JennChao/carbon-charts in…
7095c75 
…to axis-option

* 'master' of https://github.com/JennChao/carbon-charts:
  v0.37.1
  Merge pull request carbon-design-system#800 from metonym/fix-svelte-base-chart
JennChao added a commit to JennChao/carbon-charts that referenced this pull request Sep 16, 2020
@JennChao
Merge branch 'master' of https://github.com/JennChao/carbon-charts in…
12eeae0 
…to linearGradient

* 'master' of https://github.com/JennChao/carbon-charts:
  v0.38.0
  Update README.md
  Update README.md
  Update README.md
  Update README.md
  Update vue.ts
  Update angular.ts
  Update react.ts
  Update vanilla.ts
  Update README.md
  Merge pull request carbon-design-system#780 from JennChao/axis-option
  feat(core): enable or disable ruler (carbon-design-system#765)
  v0.37.1
  Merge pull request carbon-design-system#800 from metonym/fix-svelte-base-chart
theiliad pushed a commit to theiliad/carbon-charts that referenced this pull request May 17, 2021
@metonym
Merge pull request carbon-design-system#800 from metonym/fix-svelte-b…
b0344e9 
…ase-chart

* fix(svelte): rewrite BaseChart to reload chart instance

Fixes carbon-design-system#799

* refactor(svelte): remove unnecessary afterUpdate method

* chore(svelte): keep the PR scoped
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theiliad theiliad theiliad approved these changes

@cal-smith cal-smith cal-smith approved these changes

@natashadecoste natashadecoste Awaiting requested review from natashadecoste

@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.

charts-svelte: cannot read property 'removeChild' of null when using <svelte:component />
3 participants
@metonym @cal-smith @theiliad