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(visualMap): handle label collides with horizontal size visualMap #20249

Merged
merged 4 commits into from
Oct 17, 2024

Conversation

WojciechKrakowiak
Copy link
Contributor

@WojciechKrakowiak WojciechKrakowiak commented Aug 12, 2024

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

It switches visualMap handle label vertical align property basing on the visual map orientation and layout.

Fixed issues

Details

Before: What was the problem?

Label for handle at minimum position collides with the handle. The reason is that the distance between label and handle is based on the handle size and it changes depending on handle position. Currently label has always verticalAlign set to "middle", which is the cause of the collision (when distance is less than the half of the label height, the items collide). For labels below, vertical align could be "top" and for labels above, vertical align could be "bottom".

before

After: How does it behave after the fixing?

The labels below maximum range in horizontal visualMap get a slight offset to avoid collision.
This solution is not perfect, as the calculation method is different for vertical and horizontal visualMap, but I see no better way for now, to both fix the issue and keep other labels positions unchanged.

Screenshot 2024-08-13 at 12 05 41

Document Info

One of the following should be checked.

  • This PR doesn't relate to document changes
  • The document should be updated later
  • The document changes have been made in apache/echarts-doc#xxx

Misc

ZRender Changes

  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

  • visualMap-layout.html – duplicated cases for color visualMap, to cover all cases for size visualMap
  • visualMap-continuous.html – added a case that gathers color and size visualMap in all positions on single chart (the case presented in examples)

Others

Merging options

  • Please squash the commits into a single one when merging.

Other information

Copy link

echarts-bot bot commented Aug 12, 2024

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

Copy link
Contributor

github-actions bot commented Aug 13, 2024

The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-20249@dd131a0

Copy link
Contributor

@Ovilia Ovilia left a comment

Choose a reason for hiding this comment

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

It seems to break down some default label positions. You may check them by running npm run test:visual and run the demos containing "visualMap".

image

@WojciechKrakowiak
Copy link
Contributor Author

It seems to break down some default label positions. You may check them by running npm run test:visual and run the demos containing "visualMap".

image

Thank you for quick review. I implemented other solution, that affects only cases, where the labels collide. I updated the description and the screenshots.

I also noticed, that visualMap-continuous displays only one case in automated tests (while the file consists of 5 tests now). I fixed that issue, by changing the container height from relative to absolute (also did some minor refactor of the test code).

BEFORE (only first test case rendered):
Screenshot 2024-08-13 at 12 02 41

AFTER (all test cases rendered):
Screenshot 2024-08-13 at 12 03 53

@WojciechKrakowiak
Copy link
Contributor Author

Better review the test file changes with the ignore of the whitespace enabled (refactor affected the indentations).

@WojciechKrakowiak
Copy link
Contributor Author

@Ovilia is there anything more I should do, or anyone I should consult with regarding these changes?

Copy link
Contributor

@Ovilia Ovilia left a comment

Choose a reason for hiding this comment

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

We sincerely appreciate your valuable contributions! We are currently running a special promotion for users who submit a Pull Request between September 25, 2024, and December 31, 2024. Eligible participants will receive a two-year enterprise membership to Baidu Comate, valued at 2000 RMB (approximately 285 USD).

To learn more about this product, please visit https://comate.baidu.com/. If you are interested in this offer, kindly send an email with the link to your Pull Request along with your Comate username and email address to [email protected] to claim your membership.

Thank you once again for your support!

@Ovilia Ovilia merged commit 3277f98 into apache:master Oct 17, 2024
1 of 2 checks passed
Copy link

echarts-bot bot commented Oct 17, 2024

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

@Ovilia Ovilia added this to the 5.5.2 milestone Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants