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

[12979] fix bug of overriding magictype buttons' title when click the… #13366

Closed
wants to merge 139 commits into from

Conversation

pingf
Copy link

@pingf pingf commented Sep 29, 2020

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

fix bug of overriding the button title of the tool box

currently, if we change the title of the magic-type button in the toolbox, it works as expected
however, if we click the button for stack view, all magic type buttons' titles revert to the default
our users just want a chart without chinese, I guess this is the final block!

Fixed issues

Details

Before: What was the problem?

to check this one, we need do sth like this

toolbox: {
               ......
                magicType: {
                    show: true,
                    type: ['line', 'bar', 'stack'],
                    title: {
                        line: 'line view',
                        bar: 'bar view',
                        stack: 'stack view',
                        tiled: 'tiled view',
                    },
                },
             ......
}      

as stack and tiled are sth like a pair, we need to set the title for the tiled

but it only works if we never click the stack button,
once we click the button for stack, all titles revert to the the default chinese ones

image

After: How is it fixed in this PR?

the old code use magicTypeLang.title as the base to override the new title
however, this is the one of fixed chinese

and now I change the logic to flip the title of stack and tiled when we click, and use model.option to get the title dynamically

newTitle = zrUtil.merge(
            {
                      stack: model.option.title.tiled,
                      tiled: model.option.title.stack
            }, model.option.title);

and now it works as expected

image

Usage

Are there any API changes?

  • The API has been changed.

Related test cases or examples to use the new APIs

NA.

Others

Merging options

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

Other information

susiwen and others added 30 commits December 14, 2019 12:42
Merge some commits from apache/master
Merge some commits from apache/master.
Merge some commits from apache/master.
Merge some commits from apache/master
separated add husky pre-commit task from the others
Merge some commits from apache/master
fix(tooltip): added dispose method for rich tooltip, close apache#12607.
Merge some commits from apache/master.
@echarts-bot
Copy link

echarts-bot bot commented Sep 29, 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.

@susiwen8
Copy link
Contributor

@pingf Thank you very much, could you submit this PR to next branch? Also please provide a related test case.

@pingf
Copy link
Author

pingf commented Sep 29, 2020

@susiwen8
sorry for the wrong branch, this is the first time I created the PR here,
and just followed the https://github.com/apache/incubator-echarts/wiki/How-to-make-a-pull-request#coding-standard
it seems it didn't mention the next branch.

I'll change the PR to next then

@pingf pingf changed the base branch from master to next September 29, 2020 07:34
@pingf
Copy link
Author

pingf commented Sep 29, 2020

@susiwen8
I've change the merge base from master to next, it seems it has a lot of conflicts
I guess all of these can be ignored as this PR only changes a small part of one file.

so if I want to create PR next time , which branch should I use as the base branch ? the master or next?

@susiwen8
Copy link
Contributor

@pingf since too much conflict,, It's better to create a new PR for next and close this one. Base brach you chose should be next.

@pingf
Copy link
Author

pingf commented Sep 29, 2020

arrr... the changes in next are huge, even migrated to typescripts.....
I think it really needs a new wiki page for this, even the next is not released

@susiwen8
Copy link
Contributor

susiwen8 commented Sep 29, 2020

Check it out #12926
We have 2 alpha release for 5.0 base on next. And if all things go well, 5.0 would be release in October.

@pingf
Copy link
Author

pingf commented Sep 29, 2020

@susiwen8
thx, I've test the next branch, it seems the issue 12979 is already been fixed
but during test the code, find another problem, after some clicks of the buttons in the toolbox, the chart freezes!
I'll create a issue to describe it in detail

@pingf pingf closed this Sep 29, 2020
@pingf pingf deleted the fix-12979 branch September 29, 2020 08:41
@pingf
Copy link
Author

pingf commented Sep 29, 2020

@susiwen8
oops, it seems it's not fixed in next release,
the default title changes to english, just similar as I configured (sth like 'Switch to Bar Chart')
however, I think I will create a new PR then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.