You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by GitBox <gi...@apache.org> on 2022/08/02 22:10:13 UTC
[GitHub] [echarts] MasterOdin opened a new pull request, #17464: fix(type): add alignWithLabel and interval props to AxisTick
MasterOdin opened a new pull request, #17464:
URL: https://github.com/apache/echarts/pull/17464
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [x] bug fixing
- [ ] new feature
- [ ] others
### What does this PR do?
<!-- USE ONE SENTENCE TO DESCRIBE WHAT THIS PR DOES. -->
Adds missing properties to the `AxisTickOption` interface, so that it matches the [documentation](https://echarts.apache.org/en/option.html#xAxis.axisTick).
## Details
### Before: What was the problem?
<!-- DESCRIBE THE BUG OR REQUIREMENT HERE. -->
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
We were getting a typescript error about `alignWithLabel` and `interval` when we tried setting them
### After: How does it behave after the fixing?
<!-- THE RESULT AFTER FIXING AND A SIMPLE EXPLANATION ABOUT HOW IT IS FIXED. -->
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
We no longer get a typescript error using these properties.
## Document Info
One of the following should be checked.
- [x] 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
## Others
### Merging options
- [x] Please squash the commits into a single one when merging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] echarts-bot[bot] commented on pull request #17464: fix(type): add alignWithLabel and interval props to AxisTick
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on PR #17464:
URL: https://github.com/apache/echarts/pull/17464#issuecomment-1203263140
Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout [the coding standard](https://echarts.apache.org/en/coding-standard.html) and Wiki about [How to make a pull request](https://github.com/apache/echarts/wiki/How-to-make-a-pull-request).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] MasterOdin commented on a diff in pull request #17464: fix(type): add alignWithLabel and interval props to AxisTick
Posted by GitBox <gi...@apache.org>.
MasterOdin commented on code in PR #17464:
URL: https://github.com/apache/echarts/pull/17464#discussion_r936173003
##########
src/coord/axisCommonTypes.ts:
##########
@@ -179,6 +179,8 @@ interface AxisLineOption {
interface AxisTickOption {
show?: boolean | 'auto',
+ alignWithLabel?: boolean,
+ interval?: 'auto' | number | ((index:number, value: string) => boolean),
Review Comment:
Ah, okay. We had been using `XAxisOption` and I had considered `SingleAxisComponentOption` as we had logic to toggle between `value` and `category` depending on incoming data and that had felt appropriate. I suppose we could get by by doing `CategoryAxisBaseOptions & ValueAxisBaseOption` (as opposed to doing the intersection that `XAxisOption` is doing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a diff in pull request #17464: fix(type): add alignWithLabel and interval props to AxisTick
Posted by GitBox <gi...@apache.org>.
plainheart commented on code in PR #17464:
URL: https://github.com/apache/echarts/pull/17464#discussion_r936157529
##########
src/coord/axisCommonTypes.ts:
##########
@@ -179,6 +179,8 @@ interface AxisLineOption {
interface AxisTickOption {
show?: boolean | 'auto',
+ alignWithLabel?: boolean,
+ interval?: 'auto' | number | ((index:number, value: string) => boolean),
Review Comment:
Hi, thanks for your contribution! Since `alignWithLabel` and `interval` only work for the category axis, we put these two options into [`CategoryAxisBaseOption`](https://github.com/apache/echarts/blob/master/src/coord/axisCommonTypes.ts#L139-L143) rather than the common `AxisTickOption`.
/cc @pissang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] MasterOdin commented on a diff in pull request #17464: fix(type): add alignWithLabel and interval props to AxisTick
Posted by GitBox <gi...@apache.org>.
MasterOdin commented on code in PR #17464:
URL: https://github.com/apache/echarts/pull/17464#discussion_r936173003
##########
src/coord/axisCommonTypes.ts:
##########
@@ -179,6 +179,8 @@ interface AxisLineOption {
interface AxisTickOption {
show?: boolean | 'auto',
+ alignWithLabel?: boolean,
+ interval?: 'auto' | number | ((index:number, value: string) => boolean),
Review Comment:
Ah, okay. We had been using `SingleAxisComponentOption` (discovered via looking at the exported types of components as that was our main import into our code for echarts) and I had considered `XAxisOption`, but looks like it'll have the same issue. Our logic for calculating the axis dynamically uses either a `value` or `category` based on our incoming data, so I guess we could have it return `CategoryAxisBaseOptions & ValueAxisBaseOption` instead as I guess that's the suggested usage then?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org