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 11236 integrate stack/tile button into one toggle button in toolbox #11367

Merged
merged 2 commits into from
Nov 29, 2019

Conversation

alex2wong
Copy link
Contributor

related issue #11236
new design integrate stack/tile button into one toggle button in toolbox, function spec as follows:

  • When stacked state is activated, status of stack icon would be emphasis, otherwise 'normal'.
  • When stacked state is activated, title of stack icon would be 'Tile', otherwise 'Stack'.
  • When there's no data/series for this chart, stack feature would not switch between two states.

@pissang
Copy link
Contributor

pissang commented Oct 10, 2019

Awesome! I think this can be scheduled in this version 4.5.0

@pissang pissang added this to the 4.6.0 milestone Nov 18, 2019
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 tested and ran locally. The text changed and seems to be working as expected. But the icon didn't change. Is this expected? @alex2wong

@alex2wong
Copy link
Contributor Author

I tested and ran locally. The text changed and seems to be working as expected. But the icon didn't change. Is this expected? @alex2wong

@Ovilia Yes, as the function specs mentioned.. Icon itself did not switch between stack/tiled, only the title and emphasis status changed. Of course, here's another design, switch between stack/tiled icon instead of emphasis it. How do you think about it ?

@pissang
Copy link
Contributor

pissang commented Nov 28, 2019

The current implementation:

Icon itself did not switch between stack/tiled, only the title and emphasis status changed

makes more sense. It's more similar to the other icons.

Besides this, I think the tile icon is not necessary anymore. We can remove it.

@pissang pissang merged commit 1578a37 into apache:master Nov 29, 2019
pissang added a commit that referenced this pull request Nov 29, 2019
@pissang
Copy link
Contributor

pissang commented Nov 29, 2019

@alex2wong Hi, I did some improvements based on yours in the PR #11743

pissang added a commit that referenced this pull request Nov 29, 2019
@alex2wong
Copy link
Contributor Author

@alex2wong Hi, I did some improvements based on yours in the PR #11743

thank you, @pissang. It looks much better~

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

Successfully merging this pull request may close these issues.

3 participants