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.

Sign up for GitHub

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

Jump to bottom

updating pie options panel to EUI #18579

Closed
wants to merge 4 commits into from
Closed

updating pie options panel to EUI #18579

wants to merge 4 commits into from

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Apr 25, 2018 •
edited
Loading

screenshot-localhost-5601 2018 05 03 21-18-17

@ppisljar ppisljar added review Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.0.0 v6.4.0 labels Apr 25, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

cchaos
cchaos suggested changes Apr 26, 2018
View reviewed changes
Copy link
Contributor

@cchaos cchaos 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 going to submit a PR agains yours @ppisljar with just some layout cleanup stuff. Other than that the only issues I saw were:

  1. Show tooltip switch isn't working
  2. There is a console error about supplying a string as the value to EuiFieldNumber

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cchaos
Copy link
Contributor

cchaos commented May 3, 2018

Hey @ppisljar Not sure if you saw my PR: ppisljar#4

Mainly, I'm thinking that you can go ahead and get rid of the accordion hiding the Label settings as there's plenty of room to keep these visible.

@ppisljar
Copy link
Member Author

ppisljar commented May 4, 2018

nope, i completely missed that, sorry

@ppisljar ppisljar force-pushed the ang/pieOptionsEUI branch from 52145aa to 8b45a25 Compare May 4, 2018 02:10
@ppisljar
Copy link
Member Author

ppisljar commented May 4, 2018

@cchaos i split the settings into two panels and set grow to false, to match with how other options look like (for example input controls)

there is a bug when disabling labels:
screenshot-localhost-5601 2018 05 03 21-11-54

any idea what could be causing it or how to fix that ?

@ppisljar
Copy link
Member Author

ppisljar commented May 4, 2018

should we update:

  • show tooltip per slice to show tooltip ... as it is clear what it does ?
  • show INFO icons next to top level only and truncate limit ? or could we maybe get better wording for those options ?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

timroes
timroes suggested changes May 4, 2018
View reviewed changes
Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

There is one small bug that needs to be fixed. Also I would suggest, that we create an EditorOptionPanel component somewhere in Kibana, that we'll use for these panels. That way we can later way easier change the styling of all editor panels (or e.g. add collapsing to them) or move them to a new EUI component if needed. I would e.g. already now like to make the heading smaller (to xs).

src/core_plugins/kbn_vislib_vis_types/public/components/pie_editor.js Outdated
<EuiSwitch
label="Show tooltip per slice"
checked={params.addTooltip}
onChange={this.handleUpdate('addTootip', 'checked')}
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo :-) Tootip -> Tooltip (that's why this option isn't working btw).

@cchaos
Copy link
Contributor

cchaos commented May 4, 2018

Is show tooltip more descriptive than show tooltip per slice? Or is it that tooltips don't just get added to these?

I don't think that the other two elements need info icons. They're almost self-explanatory, they're non-destructive actions and can you easily see the change after pressing play.

I don't seem to have that weird checkbox issue as you do:
screen shot 2018-05-04 at 14 20 41 pm

What browser are you using and if you go to https://elastic.github.io/eui/#/forms/form-controls and down to the checkbox section, do you have the issue there as well?

@cchaos
Copy link
Contributor

cchaos commented May 4, 2018

@ppisljar I found the source of the input bug:

screen shot 2018-05-04 at 16 20 51 pm

Looks like Kibana's [disabled] styles on inputs is overriding our hiding of this checkbox for switches.

@ppisljar
Copy link
Member Author

ppisljar commented May 8, 2018

'show tooltip' is used on every other visualization, so i suggest to keep it consistent.

@ppisljar ppisljar force-pushed the ang/pieOptionsEUI branch 3 times, most recently from d722e2c to 725ca7a Compare May 8, 2018 14:23
@ppisljar ppisljar mentioned this pull request May 8, 2018
Closed
@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor

timroes commented May 10, 2018

Blocked by elastic/eui#805

@snide snide mentioned this pull request May 10, 2018
Merged
@snide snide removed the blocked label May 10, 2018
ppisljar and others added 4 commits May 14, 2018 20:46
@ppisljar
updating pie options panel to EUI
5839c13
@cchaos @ppisljar
Tweaks to layout
7178bd4 
- Removed the accordion (unecessary as there is still a lot of space available)
- Added section headers
- Wrapped in a EuiPanel
- Disabling all other label setting controls if the “Show labels” is off
- Changing all labels to sentence case (only first word should be capitalized)
@ppisljar
splitting into two tabs
5d6e6c3
@ppisljar
rebasing
2fc48fc
@ppisljar ppisljar force-pushed the ang/pieOptionsEUI branch from 725ca7a to 2fc48fc Compare May 15, 2018 01:48
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ppisljar
Copy link
Member Author

an experiment where ng-show was replaced by ng-if:

#19846

@ppisljar ppisljar mentioned this pull request Jun 13, 2018
Closed
@ppisljar
Copy link
Member Author

closing, as the bug in eui will not be fixed and we will not be euifying our editor for now

@ppisljar ppisljar closed this Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timroes timroes timroes requested changes

@cchaos cchaos cchaos requested changes

@thomasneirynck thomasneirynck Awaiting requested review from thomasneirynck

@alexfrancoeur alexfrancoeur Awaiting requested review from alexfrancoeur

Assignees
No one assigned
Labels
blocked Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. v6.4.0 v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ppisljar @elasticmachine @cchaos @timroes @snide