Update chart size on render finished

[afzalsayed96]

I was playing around and found an issue related to chart layout when performing client side navigation

Screen.Recording.2022-11-03.at.9.59.34.PM.mov

I'm new using echarts so not sure if this would be the most idiomatic solution to this problem. Happy to hear your thoughts

[changeset-bot]

🦋 Changeset detected

Latest commit: ee0f83d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@evidence-dev/components	Patch
@evidence-dev/evidence	Patch
evidence-test-environment	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
evidence-docs	✅ Ready (Inspect)	Visit Preview	Nov 11, 2022 at 4:49PM (UTC)

[hughess]

@afzalsayed96 thanks for looking into this!

I tried your changes and got the resizing working when I use client-side navigation, but ran into a couple of issues - do these also show up on your end, or are you seeing different behaviour?

• The chart gets into the correct width eventually, but still starts in the extra-wide position

  • In the long-run, we might need to adjust how we are initializing and loading the charts, but I think this solution is better than what we have currently (where the only solution is to refresh the page, as your screen recording shows)

• The "finished" event looks like it continues to fire after the chart has finished rendering

  • I added console.log("resizing") to the resize function to see how often it was firing

[afzalsayed96]

@hughess It seems the root cause of the issue as pointed in apache/echarts#11791 is related to flex layout of the container. And the solution is to observe the flex child for resize events.

While, this solution does seem better than the previous one, it relies on the assumption that class 'chart-container' would be always applied. Let me know what you think of it.

[hughess]

@afzalsayed96 this seems to work perfectly! Nice solution.

I don’t think there’s an issue with relying on the chart container since it should always be there.

There are two last pieces to think about on this:

1. This seems to have shut off the initial load animation of the chart, but has retained the animation involved in hovering on chart elements and displaying tooltips. Funnily enough, that’s actually what we wanted to have initially but echarts only offers one animation toggle in their config. So no change needed here, but something for us to be aware of in case it’s indicative of another issue.
2. I’d like to test this with a chart that’s less than 100% width. Eventually we’ll offer the ability to put multiple charts across the width of a page, so I want to make sure this wouldn’t cause any issues with that.

[hughess]

@afzalsayed96 I've tested with some charts I set up to be less than 100% width and it works well.

After taking a closer look, I have question about this line:
const chartContainerElement = document.getElementsByClassName('chart-container')[0]

If a page has multiple charts, this will only select the first chart on the page - is that okay in this case because we only need to observe one chart on the page to effectively trigger the resizing for all the charts?

[afzalsayed96]

If a page has multiple charts, this will only select the first chart on the page - is that okay in this case because we only need to observe one chart on the page to effectively trigger the resizing for all the charts?

Yes, incidentally this would have worked, as when one chart-container would resize due to it's parent container, it's siblings would resize as well. But it wasn't that clean after all.

Hence, I've switched the logic to observe the parent article element which triggers resize of it's children chart-container elements. This will have the same effect and is more explicit.

[hughess]

That's perfect. I'd say this is good to go! Going to merge into main.

Thanks again for this contribution @afzalsayed96!!

[archiewood]

Hey @afzalsayed96,

Was just playing around with a site on mobile (iOS, Firefox, Evidence v5.0.16) and noticed that on some charts data were not being loaded until I interacted with the page:

RPReplay_Final1670370116.mp4

Platforms:

• Doesn't seem to be an issue on macOS Firefox, Chrome
• Replicated error on macOS Safari
• Replicated error on iOS Chrome, Safari

Could this be related the this PR, do you think?

[afzalsayed96]

Hey Archie, Let me take a look at this later tonight and get back to you

…

On Wed, Dec 7, 2022 at 10:08 AM Archie ***@***.***> wrote: Hey @afzalsayed96 <https://github.com/afzalsayed96>, Was just playing around with a site on mobile (iOS, Firefox), and noticed that some charts were not being loaded until I interacted with the page: https://user-images.githubusercontent.com/58074498/206089628-8c5d51c2-a81e-4aae-a36e-0df1dc84e704.mp4 Could this be related the this PR, do you think? — Reply to this email directly, view it on GitHub <#450 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADLBEO5DEVWDNE3RO246AUDWMAIEHANCNFSM6AAAAAARWKQCYM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[archiewood]

Source code I used to generate this:
https://github.com/evidence-dev/demo/tree/add-customer-reporting-example

Deploy preview:
https://deploy-preview-6--evidence-demo.netlify.app/partners