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

Charts: Changing the chart type doesn't remove old class name #3144

Closed
kvaldillez opened this issue Nov 19, 2019 · 9 comments · Fixed by #4965
Closed

Charts: Changing the chart type doesn't remove old class name #3144

kvaldillez opened this issue Nov 19, 2019 · 9 comments · Fixed by #4965
Assignees
Labels
type: bug 🐛 [2] Velocity rating (Fibonacci)

Comments

@kvaldillez
Copy link

You can reproduce locally by initializing a chart with a pie chart type and then calling the .chart(settings) with a different chart type and a different dataset (optional). You can also reproduce on the IDS demo by:

  1. Going to: https://design.infor.com/code/ids-enterprise/latest/demo/components/pie/example-
  2. Open console and enter:
    var areaData = [{ data: [{ name: 'Component D', value: 10.1, id: 'A', tooltip: 'Component D <b>{{percent}}</b>' }, { name: 'Component E', value: 12.2, id: 'B', tooltip: 'Component E <b>{{percent}}</b>' }] }];
    $('#pie-chart-example').chart({type: 'area', dataset: areaData});
  3. Observe that the chart-container class still has chart-pie even though it is no longer a pie chart.

Expected: chart-pie class to be removed and replaced by new chart type class (line-chart in this case).

@tmcconechy tmcconechy added [2] Velocity rating (Fibonacci) type: bug 🐛 labels Nov 19, 2019
@tmcconechy
Copy link
Member

Seems like the destroy of chart is not doing removing the class.

@kvaldillez
Copy link
Author

Yes, I tried doing a manual .destroy() on it and then running .chart(settings) again which ended in the same result of the previous class still being there.

@tmcconechy
Copy link
Member

yeah i think the chart "reseting" is also calling destroy. So that's where the fix would lie.

@tmcconechy tmcconechy changed the title Changing the chart type doesn't remove old class name Charts: Changing the chart type doesn't remove old class name Nov 19, 2019
@kvaldillez
Copy link
Author

FYI - As a workaround, I'm removing all classes from the container and then adding back in chart-container prior to calling .chart(settings) which then adds the correct chart class. Not ideal but it works.

@tmcconechy
Copy link
Member

Yeah that works. I think our code would individual destroy each component (chart) and each should strip its class.

@tmcconechy
Copy link
Member

Descoping some older issues we didnt get a chance to complete.

@tfavorite
Copy link
Contributor

@tmcconechy we'd still like this addressed at some point. What's the correct way to keep this in your backlog?

@tmcconechy tmcconechy reopened this Mar 1, 2021
@tmcconechy
Copy link
Member

Ok, For more details:

We can fix it but i was thinking that going forward it would be individual charts so you won't be able to switch the type depending on chart. For example pie and donut is probably a setting but pie to bar is a seperate chart. But bar to bar grouped is the same chart ect...

So you will in the future have to just invoke one or the other and hide/show. But the fix for this in the meantime is fairly simple so can do it but going forward it wont be possible anymore. (Also the classes wont matter so much anyways)

@janahintal
Copy link
Contributor

This issue is now resolved.
Verified in https://master-enterprise.demo.design.infor.com/components/pie/example-index.html

image
image
image

@janahintal janahintal moved this from Ready for QA (master) to Done in Enterprise 4.50.x (Mar 2021) Sprint Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 [2] Velocity rating (Fibonacci)
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants