Skip to content

feat: Allow user to use interval on time axis#17044

Closed
jiawulin001 wants to merge 3 commits intoapache:masterfrom
jiawulin001:issue#16927
Closed

feat: Allow user to use interval on time axis#17044
jiawulin001 wants to merge 3 commits intoapache:masterfrom
jiawulin001:issue#16927

Conversation

@jiawulin001
Copy link
Copy Markdown
Member

@jiawulin001 jiawulin001 commented May 17, 2022 •
edited
Loading

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Allow user to use interval to specify the interval between ticks on time axis.

Fixed issues

Details

Before: What was the problem?

User-specified interval does not work even after min and max is speicified.

#16905 (interval = 12 hours) #16927 (interval = 10 minutes)
before#16905 before#16927

After: How is it fixed in this PR?

User-specified interval is taken in and will overwrite default interval if specified.

#16905 (interval = 12 hours) #16927 (interval = 10 minutes)
after #16905 after#16927

Misc

  • The API has been changed (apache/echarts-doc#xxx).
  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

axis-inerval.html

Others

Merging options

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

Other information

@echarts-bot
Copy link
Copy Markdown

echarts-bot bot commented May 17, 2022

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.

@pissang pissang added this to the 5.3.3 milestone May 17, 2022
@pissang pissang requested a review from Ovilia May 17, 2022 07:03
@hanqicheng
Copy link
Copy Markdown

hanqicheng commented Jun 2, 2022

image
After testing, I found that the current default value seems to be fixed at 60 minutes, and customization is still not supported. The following is my test screenshot. Please point out any errors in use

@jiawulin001
Copy link
Copy Markdown
Member Author

jiawulin001 commented Jun 7, 2022

After testing, I found that the current default value seems to be fixed at 60 minutes, and customization is still not supported.

I suppose this is under the 5.3.2 environment and it would not allow user to customize time interval. At this version user would have to specify minInterval to directly adjust it or change 'splitNumber' to indirectly adjust it. And the new interval in both cases is rounded to some fixed interval here:

echarts/src/scale/Time.ts

Line 371 in 79fdbe4

function getDateInterval(approxInterval: number, daysInMonth: number) {

@Ovilia Ovilia modified the milestones: 5.3.3, 5.4 Jun 10, 2022
Ovilia
Ovilia requested changes Jun 20, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@Ovilia Ovilia left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this PR requires a lot more test cases to test if it works as expected for different user given interval and the data. You can add a set of test cases under timeScale-formatter.html.

src/coord/axisHelper.ts
// FIXME
if (interval != null) {
(scale as IntervalScale).setInterval && (scale as IntervalScale).setInterval(interval);
//If interval is set by user, mark it as true
Copy link
Copy Markdown
Contributor

@Ovilia Ovilia Jun 20, 2022

Choose a reason for hiding this comment

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

Space before If

src/scale/Interval.ts
* @param interval Interval Set by user, not used here but inheritated by TimeScale
*/
calcNiceTicks(splitNumber?: number, minInterval?: number, maxInterval?: number): void {
calcNiceTicks(splitNumber?: number, minInterval?: number, maxInterval?: number, interval?: number): void {
Copy link
Copy Markdown
Contributor

@Ovilia Ovilia Jun 20, 2022

Choose a reason for hiding this comment

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

It seems interval is not used here?

src/scale/Time.ts
const extent = this._extent;
const span = extent[1] - extent[0];
this._approxInterval = span / approxTickNum;
//Use set interval if specified by user
Copy link
Copy Markdown
Contributor

@Ovilia Ovilia Jun 20, 2022

Choose a reason for hiding this comment

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

Space before Use

src/scale/Time.ts
case 'quarter':
case 'month':
interval = getMonthInterval(approxInterval);
//Use interval set by user if specified
Copy link
Copy Markdown
Contributor

@Ovilia Ovilia Jun 20, 2022

Choose a reason for hiding this comment

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

Space

@Ovilia
Copy link
Copy Markdown
Contributor

Ovilia commented Jul 27, 2022

Another problem for this PR is, for monthly labels, the intervals are not the same. Some months have 30 days while others may have 31, 28 or 29 days. If the developer want to use monthly interval, setting the interval to be 1000 * 3600 * 24 * 30 won't help.

@Ovilia Ovilia mentioned this pull request Aug 26, 2022
Open
@Ovilia Ovilia removed this from the 5.4 milestone Sep 1, 2022
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 31, 2024

This PR has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you wish not to mark it as stale, please leave a comment in this PR. We are sorry for this but 2 years is a long time and the code base has been changed a lot. Thanks for your contribution anyway.

@github-actions github-actions bot added the stale Inactive for a long time. Will be closed in 7 days. label Aug 31, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 7, 2024

This PR has been automatically closed because it has not had recent activity. Sorry for that and we are looking forward to your next contribution.

@github-actions github-actions bot closed this Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ovilia Ovilia Ovilia requested changes

Assignees

No one assigned

Labels

size/M stale Inactive for a long time. Will be closed in 7 days.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

当x轴为时间轴时X轴配置项interval设置无效[Bug] xAxis don't display as setted values when type :"time" [Bug]

4 participants

@jiawulin001 @hanqicheng @Ovilia @pissang