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

Aria omit columns #20038

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Aria omit columns #20038

wants to merge 9 commits into from

Conversation

semla
Copy link

@semla semla commented Jun 17, 2024

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Allows omitting columns from the data in aria-label.

Fixed issues

Details

Before: What was the problem?

All data was present in the aria-label. My use case was for a map chart, where the coordinates should be excluded.

After: How does it behave after the fixing?

Add column indexes to be excluded to aria.data.columnsToExclude and they will not appear in the data in aria.label.

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 Jun 17, 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.

Document changes are required in this PR. Please also make a PR to apache/echarts-doc for document changes and update the issue id in the PR description. When the doc PR is merged, the maintainers will remove the PR: awaiting doc label.

Copy link
Contributor

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

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.

I'm a little confused about the purpose of this PR. At first glance, I thought this is to ignore some dimensions (i.g.: data: [[100, 100, 2], [200, 300, 4]] for scatters but display only 2 and 4 in aria-label), but based on current implementation, with columnsToExclude: [0, 1], it seems to filter the first and seconda data instead of the first and second dimension of all data.

I tested with the following code:

chart.setOption({
    aria: {
        enabled: true,
        series: {
            multiple: {
                prefix: '{seriesCount}个图表系列组成了该图表。'
            }
        },
        data: {
            columnsToExclude: [1]
        }
    },
    tooltip : {
        trigger: 'axis',
        axisPointer : {            // 坐标轴指示器,坐标轴触发有效
            type : 'shadow'        // 默认为直线,可选为:'line' | 'shadow'
        }
    },
    legend: {
        data:['热度', '正面', '负面']
    },
    grid: {
        left: '3%',
        right: '4%',
        bottom: '3%',
        containLabel: true
    },
    xAxis : [
        {
            type : 'value'
        }
    ],
    yAxis : [
        {
            type : 'category',
            axisTick : {show: false},
            data : ['汽车之家','今日头条','百度贴吧','一点资讯','微信','微博','知乎']
        }
    ],
    series : [
        {
            name:'热度',
            type:'bar',
            label: {
                normal: {
                    show: true,
                    position: 'inside'
                }
            },
            data:[300, 270, 340, 344, 300, 320, 310]
        },
        {
            name:'正面',
            type:'bar',
            stack: '总量',
            label: {
                normal: {
                    show: true
                }
            },
            data:[120, 102, 141, 174, 190, 250, 220]
        },
        {
            name:'负面',
            type:'bar',
            stack: '总量',
            label: {
                normal: {
                    show: true,
                    position: 'left'
                }
            },
            data:[-20, -32, -21, -34, -90, -130, -110]
        }
    ]
});

let data = seriesModel.getData();
const columnsToExclude = labelModel.get(['data', 'columnsToExclude']);
if (columnsToExclude) {
data = data.filterSelf(idx => !columnsToExclude?.includes(idx));
Copy link
Contributor

Choose a reason for hiding this comment

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

This will make the elements in the chart not rendered. What we want is only to change the aria-label instead of the original chart.

Copy link
Author

Choose a reason for hiding this comment

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

I wanted to do the filtering before the check with maxDataCnt - but of course using filterSelf was all wrong.
I have switched back to the old solution in commit 77580e9.
Regarding maxDataCnt, I guess the length of the array columnsToExclude could be added to maxDataCnt to compensate for the excluded columns:

if (columnsToExclude) {
        maxDataCnt += columnsToExclude.length;
}

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

What kind of filtering does this PR want to provide? Do you want to hiding some bars or only eliminate some dimensions from aria-label (in which case you shouldn't call filterSelf because it changes the data itself)?

Copy link
Author

Choose a reason for hiding this comment

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

What kind of filtering does this PR want to provide? Do you want to hiding some bars or only eliminate some dimensions from aria-label (in which case you shouldn't call filterSelf because it changes the data itself)?

Only eliminate some dimensions form aria-label. So filterSelf was a mistake .

@semla semla marked this pull request as draft June 19, 2024 17:05
const dataLabels = [];
for (let i = 0; i < data.count(); i++) {
if (i < maxDataCnt) {
if (i < maxDataCnt && !columnsToExclude?.includes(i)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i is the index of data. How can we filter certain dimensions by checking index of data? The current code is more like dataIndexToExclude than columnsToExclude.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, how about bcd1a81 instead?

@Ovilia
Copy link
Contributor

Ovilia commented Jun 21, 2024

Please make this PR "ready for review" when you are ready so that we know it's time for us to review. Thanks!

@Ovilia
Copy link
Contributor

Ovilia commented Jun 28, 2024

Is this PR ready for review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: awaiting doc Document changes is required for this PR. PR: first-time contributor size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants