Skip to content

Conversation

@ggggg
Copy link
Contributor

@ggggg ggggg commented Nov 25, 2022

Motivation and Context

This fixes a small visual bug (see #6351)

Your Changes

Description:
Disabled overflow scrolling for the data-chart-container class as it is not needed and there is no overflow.

Type of change (select all that apply):

  • Bug fix (non-breaking change which fixes an issue)

Testing

Questions and Comments (if applicable)

Checklist

  • I have performed a self-review of my own code.
  • I have verified that the pre-commit.ci checks have passed.
  • I have verified that the CI tests have passed.
  • I have reviewed the test coverage changes reported on Coveralls.
  • I have updated the Changelog.md file.

Pull request to make documentation changes (if applicable)

@ggggg ggggg marked this pull request as ready for review November 25, 2022 23:27
@mishaschwartz
Copy link
Contributor

@ggggg Thank you for this PR. I was able to replicate this issue and your fix looks like it works to solve the flickering problem.
However, I think that the problem is actually caused by a different part of the code. If you look at this line:

https://github.com/MarkUsProject/Markus/blob/v2.1.6/app/assets/javascripts/Components/Helpers/data_chart.jsx#L60

There's the bit where it's setting the style of the canvas rendered by the Bar component to display: "inline-flex". I think that this is a holdover from before we were using the react-chartjs-2 library to render charts. If we remove this so that the line is simply:

style={{margin: "10px"}}

Then the problem is fixed as well.

I think what is happening is that setting the display to "inline-flex" permits the browser to resize the canvas element. But the react-chartjs-2 library is also dynamically resizing the canvas element and so it flickers back and forth since the browser and the js library have a slightly different idea of what the canvas size should be.

Setting overflow to hidden works as well because I think it prevents the browser from triggering a resize since it just hides the extra content instead. However, I'd rather fix the content at the "source" by not adding the unnecessary display: "inline-flex" style in the first place.

Does that make sense to you? Do you mind trying out both solutions and confirming whether or not they work for you as well?

Thanks!

@mishaschwartz
Copy link
Contributor

@ggggg I'd like to include this in the next release, scheduled for the end of the week. Please let me know if you have any concerns with my suggestions, otherwise I'm going to resolve this in the next few days.

@ggggg
Copy link
Contributor Author

ggggg commented Dec 6, 2022

Yeah, I think it works too!

@mishaschwartz
Copy link
Contributor

thanks @ggggg I'll make sure this gets into the next release. And thanks again for the bug report!

@mishaschwartz
Copy link
Contributor

@ggggg would you like to be added to the contributor's list? If so, what name should I add?

@ggggg
Copy link
Contributor Author

ggggg commented Dec 7, 2022

I would love that, my full name is Ido Ben Haim.
Thank you!

@david-yz-liu david-yz-liu merged commit 2455553 into MarkUsProject:master Dec 11, 2022
@david-yz-liu
Copy link
Collaborator

Thanks @ggggg!

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.

Minor visual bug - popup animation bugging when clicking "Marks Chart"

3 participants