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(custom): optionally return category from value(). close #12952 #13715

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LemniscateX
Copy link

Signed-off-by: LemniscateX [email protected]

Brief Information

This pull request is in the type of:

  • bug fixing

What does this PR do?

Fix value() method of parameter api in custom renderItem from returning unexpected ordinal index.

Fixed issues

Fix #12952

Details

Before: What was the problem?

RenderItem's data construction will use the ordinal index of dimension x instead of its raw value. Current value() implementation will return this ordinal index.

After: How is it fixed in this PR?

Add detect logic to value() to check if dim is dimension x. If so, return its raw value from ordinalMeta.categories.

@echarts-bot
Copy link

echarts-bot bot commented Nov 28, 2020

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.

@pull-request-size pull-request-size bot added size/S and removed size/M labels Nov 28, 2020
@pissang pissang requested a review from 100pah November 29, 2020 12:32
@pissang pissang added this to the 5.1.0 milestone Dec 14, 2020
Comment on lines 1710 to +1711
dataIndexInside == null && (dataIndexInside = currDataIndexInside);
return data.get(data.getDimension(dim || 0), dataIndexInside);
dim == null && (dim = 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

First time I learnt that '&&' can be used this way!

@100pah 100pah self-assigned this Mar 1, 2021
@100pah
Copy link
Member

100pah commented Mar 22, 2021

@pissang @LemniscateX

I am not sure about the behavior of this API value().

Current behavior:

  • If the dimension is in type of 'ordinal', and the dimension is used by an 'category' axis, value() returns the ordinal number rather than the original category string.
  • Else value() returns the original category string.

If we change the behavior like the PR did:
In all cases value() returns the original category string.
The two issues need to be considered.

  1. It is a break change. And in some cases that might not easy to fix that, because users might not know how to get the ordinal number.
  2. As 1 mentioned, users might need to get the ordinal number, which might be used to calculate the coordinate or retrieve something as an index.

So I prefer not to modify the behavior of value().

But since 5.0 there is an extra api ordinalRawValue() provided, but not documented, but used in an test/custom-shape-morphing3.html. The behavior is:

  • If the original value is 'hahaha' and the dimension type is 'ordinal', rawValue() always returns 'hahaha' whether this dimension is used by a category axis or not.
  • In other cases, it is the same as value() did.

Is ordinalRawValue() it enough for this issue? Or anything else ideas?

@LemniscateX
Copy link
Author

@pissang @LemniscateX

I am not sure about the behavior of this API value().

Current behavior:

  • If the dimension is in type of 'ordinal', and the dimension is used by an 'category' axis, value() returns the ordinal number rather than the original category string.
  • Else value() returns the original category string.

If we change the behavior like the PR did:
In all cases value() returns the original category string.
The two issues need to be considered.

  1. It is a break change. And in some cases that might not easy to fix that, because users might not know how to get the ordinal number.
  2. As 1 mentioned, users might need to get the ordinal number, which might be used to calculate the coordinate or retrieve something as an index.

So I prefer not to modify the behavior of value().

But since 5.0 there is an extra api ordinalRawValue() provided, but not documented, but used in an test/custom-shape-morphing3.html. The behavior is:

  • If the original value is 'hahaha' and the dimension type is 'ordinal', rawValue() always returns 'hahaha' whether this dimension is used by a category axis or not.
  • In other cases, it is the same as value() did.

Is ordinalRawValue() it enough for this issue? Or anything else ideas?

I agree with you that ordinalRawValue() can meet the demand of getting original value no matter what the data type it is, and as well as to fix the issue.

It just seems that value() is out of users' expectation when it comes to dimension x. I guess a little bit mention in document would be better. XD

@pissang pissang modified the milestones: 5.1.0, 5.x May 6, 2021
@Ovilia Ovilia modified the milestones: 5.x, TBD Sep 1, 2022
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.

自定义系列,数据第一个维度为ordinal类型,api.value()读取时值不正确
5 participants