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(util): properly assign helper-svg styles #657

Merged
merged 1 commit into from Jul 11, 2022
Merged

Conversation

nikku
Copy link
Member

@nikku nikku commented Jul 6, 2022

@barmac
Copy link
Member

barmac commented Jul 11, 2022

I can't reproduce the original issue on Mac. @philippfromme @Skaiir one of you could probably help out here.

@barmac barmac requested review from Skaiir and removed request for barmac July 11, 2022 06:51
@nikku
Copy link
Member Author

nikku commented Jul 11, 2022

Interesting that this is not an issue on MacOS? I guess you're also using latest Chrome to test?

You should however be able to reproduce the fact that the original code (fixed) does not do what it is supposed to do.

Assigning visibility: none to a DOM element does nothing. This works for SVG only, sometimes.

@barmac
Copy link
Member

barmac commented Jul 11, 2022

Interesting that this is not an issue on MacOS? I guess you're also using latest Chrome to test?

I checked once more on the demo site and I could reproduce the problem to some extent. It required that I scrolled while hovering the text on the bottom or the bpmn.io logo.

Scrollbars are not normally visible on Mac, but they appear when I initiate scrolling. This was possible before your changes and impossible after they were applied.

Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

This fixes the issue. Each of the style properties is required for the fix to work.

@fake-join fake-join bot merged commit 911909b into develop Jul 11, 2022
@fake-join fake-join bot deleted the helper-svg-fix branch July 11, 2022 07:07
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jul 11, 2022
@barmac
Copy link
Member

barmac commented Jul 11, 2022

Hmm this could have been merged to master.

@nikku
Copy link
Member Author

nikku commented Jul 11, 2022

🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants