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(Axis): xAxis axisTick custom interval funciton work error with boundaryGap false #20436

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sz-p
Copy link
Contributor

@sz-p sz-p commented Oct 22, 2024

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Fixed issues

#20424

Details

Before: What was the problem?

image

After: How does it behave after the fixing?

image

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

N.A.

Others

Merging options

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

Other information

Copy link

echarts-bot bot commented Oct 22, 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 Oct 22, 2024

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

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.

image

I think the correct position of the tick should be placed at the red position. Is that right? Run demo

@sz-p
Copy link
Contributor Author

sz-p commented Oct 22, 2024

Thank you for reviewing!

I see the problem. and i found a new problem.

If every one tick in intervalList is true. the last one will be out of control.

image

The new fix way is change tick to right position and add last one after intervalList.

See the result after fix.

image

@Ovilia
Copy link
Contributor

Ovilia commented Oct 23, 2024

Thanks for the detailed test!

Can you explain why there is a tick before Thu when the last is false in the last two charts?

What's the current policy for the last tick to show?

@sz-p
Copy link
Contributor Author

sz-p commented Oct 23, 2024

The current logic is simple: add last one after intervalList.

To form a tick pair, similar to the effect of boundaryGap: true, add an ending tick to the intervalList.

A potential issue arises if the intervalList is [true, false]. By adding an end tick, it becomes [true, true], which might seem odd.

A more intelligent way is if the tick is end of axis then add it. if not ignore it.

Should we use this?

@Ovilia
Copy link
Contributor

Ovilia commented Oct 24, 2024

I don't think axis ticks work in pairs, so I'd think add last one after intervalList works pretty intuitive to me.

@sz-p
Copy link
Contributor Author

sz-p commented Oct 24, 2024

See new reault.

image

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.

My expected behavior the the last tick to be show or not would be:

[true, true, true, true]: true
[false, false, false, false]: false
[true, false, false, false]: true
[true, false, true, true]: true
[true, false, true, false]: true
[false, true, true, true]: true

The logic is, the last one is not displayed only if none ticks is displayed. But I'm open to what you think. Please let me know your opinions on this and let's discuss about what's more intuitive.

@sz-p
Copy link
Contributor Author

sz-p commented Oct 29, 2024

See new reault.

image

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