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

fix(legacy-charts): fix render multiple charts at one page #803

Merged
merged 2 commits into from Apr 7, 2022

Conversation

thabarbados
Copy link
Collaborator

Fix or Enhancement?

fix the error, when multiple charts can't render on one page
from issues #800 #801

  • All tests passed

Environment

  • OS: BigSur MacOs
  • NPM Version: 8.5.1

@github-actions
Copy link

github-actions bot commented Apr 6, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/index.cjs 1.16 KB (0%) 24 ms (0%) 94 ms (+7.39% 🔺) 118 ms
dist/index.js 1.04 KB (0%) 21 ms (0%) 136 ms (+53.22% 🔺) 157 ms

export function generateChart(chartId, chartType, chartController) {
let _chart = null
let _store = null
Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks like "ref", so then maybe just

let _chartRef = { current: null }
...
_chartRef.current

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dangreen, I checked, and your solution doesn't fix this bug.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, here just let _chartRef = null

_store = new ComponentStore() -> _chartRef = { current: null }
_store.setComponentChart(...) -> _chartRef.current = ...
etc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It works. I used it.

fix the error, when multiple charts can't render on one page

fix #800 #801
Comment on lines 86 to 88
_chartRef !== null &&
'current' in _chartRef &&
_chartRef.current !== null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
_chartRef !== null &&
'current' in _chartRef &&
_chartRef.current !== null
_chartRef?.current

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

used

Comment on lines 103 to 104
_chartRef !== null &&
'current' in _chartRef
Copy link
Collaborator

Choose a reason for hiding this comment

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

...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

@dangreen dangreen merged commit fe97040 into main Apr 7, 2022
@dangreen dangreen deleted the fix-multiple-legacy branch April 7, 2022 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants