-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Lens] Add styling options for x and y axes on the settings popover #71829
Conversation
@AlonaNadler @cchaos I would like your feedback on the styling settings of X-axis for Lens. It has to do with this issue. Here is a demo: and here is a screenshot of the settings: What do you think? Do you have any suggestions regarding the UI? |
@elasticmachine merge upstream |
Thank you for adding these options! A couple first impressions.
|
Thank you @cchaos for your feedback! Totally agree with the third, was thinking the same. Waiting for your mock, I understood your idea but the mock will help a lot! And thanx for the tip about the positive tense :) |
That was quick :D Will start working on this and let you know if I encounter any problem |
Thanks @stratoula, I checked the PR
|
I had the same discussion in my head @AlonaNadler , but what ends up happening is that it gets complicated with multiple x and y axis configs. Take the example from @stratoula's gif: There's 2 x-axis and 2 y-axis configs each with their own "Label" input. How does the chart decide and how does the user know which label will be shown on the chart? So I see two options for handling this.
Edit Though I'm now realizing that the config labels also label them in the legend, so I don't think we can go with route 2. |
Eventually we really need to be able to edit everything directly on the chart, like double-click the axis title to change it. |
Good call, Caroline - I will create an issue for this in elastic-charts as I think it's not possible at the moment. |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@AlonaNadler And I talked about this today and we're both in the mindset that we need to stop separating styling concerns from their configuration. This means that any settings under the toolbar needs to be Chart-type specific and not affected by the configuration. The split causes too much dissonance and makes the settings harder to find. Yes, this may cause some complexity on the engineering side as to how to handle some specific cases, but I've outlined how this would be handled in this mock: (Use your arrow keys to scroll through the screens) Essentially, there are 2 main ideas:
Those ideas make it also a lot easier to understand how to handle the styling if there is both a left and right axis. The current PR does not handle this situation. |
@elasticmachine merge upstream |
@cchaos We went through the new UI setup and I have some concerns with this approach:
AlternativesI think there is an alternative to this to ease the tension which is going into the other direction - move settings which are applied on a chart level into the toolbar and specifically make them chart level settings. IMHO this is clearer because it doesn't have the problem of specifying the same setting in
My concerns are not about the technical complexity (it's easy to implement "first one takes precedence"), it's about worrying we put controls in a place they don't belong. I might have not considered something here, happy to discuss this further. |
The complexity by adding another place to set axis label doesn't worth the benefit to allow remove the axis labels (which is a niche)
minor in my opinion chart setting is a good idea in my opinion and we can add the grid lines there but users shouldn't have to go to chart setting to name the x-axis y-axis labels - which is a common operation. There are many different chart configurations we can have add to chart configuration in the future. |
That’s exactly how it works on this PR (as far as I understood) - if the override on chart level isn’t used, the labels of the first x and y dimensions are used. I would be fine with this behavior (especially as it’s compatible with the current UI of Lens. So they wouldn’t have to switch the menu to do this in most cases, but there is a top level setting they can use for full control (e.g. hiding it completely). We could put it in an advanced section as well to prevent it from being used often. Another option I can think of is to not allow the user to overwrite the axis labels from the chart level settings, only hiding them (as an "advanced feature" used from time to time). Labels are always tied to a dimension, global settings are specified in the chart settings. What do you think @AlonaNadler ? |
After syncing offline with @AlonaNadler we came to the conclusion doing the discussed changes is not worth the effort at the moment and we will stick with top level axis options for 7.10 given the existing experience with defining axis labels via the first dimension will be kept for now. @cchaos given this are you fine with the current state of the PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks mostly good to me, I have some technical nits
x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx
Outdated
Show resolved
Hide resolved
setYaxistitle(yTitle); | ||
}, [xyTitles]); | ||
|
||
const [tickLabelsVisibilitySettings, setTickLabelsVisibilitySettings] = useState({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you storing all of these settings both in local state and in the frame state (via setState
)? When you call setState
, the frame will re-render the toolbar and value will be adjusted. We should try to avoid local state whenever possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly, without localState is extremely slow, have to check it again though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, and an interesting solution.
For the buttons it should be irrelevant, for the label in the dimension popover we used this workaround (basically debouncing the state update):
kibana/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/popover_editor.tsx
Line 59 in b165538
const LabelInput = ({ value, onChange }: { value: string; onChange: (value: string) => void }) => { |
It keeps the ugly performance optimization in a single place, so the top level form component doesn't have to deal with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the localstate from gridlines and ticklabels
💚 Build SucceededBuild metricspage load bundle size
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, works as expected. Thanks for working on this!
@cchaos Could you have another look? |
There's still the question of how to handle right vs left Y-axis grid lines and tick marks. |
Good point, @cchaos - I assumed we would implement that in a separate PR as the current status is a meaningful increment and benefits users, so there's no need to keep this PR open longer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Ok, so long as we do circle back before the 7.10 release. I'm good with this PR as-is to get this functionality started. Thanks for all your work @stratoula and sorry about all the extra discussion.
…lastic#71829) * [Lens] Add styling options for x axis on the settings popover * ts related changes * Changes to the popover's design and y-axis implementatin * fix types and add unit tests * Add extra translations * Fix functional test and change the logic of the yTitle * fixes * fix showTitle settings bug * Fix ticklabels bug on y axes * fix some tests * Change the user flow on x and y titles on settings popover and enable the gridlines by default * disable linter warning * PR Comments * Add a comment to callback to explain the decision to listen only to open changes Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
I will create a separate issue for left/right y axis settings. |
* master: (106 commits) [Functional Tests] Adds a wait time between setting the index pattern and the time field on TSVB (elastic#74736) [Lens] Add styling options for x and y axes on the settings popover (elastic#71829) [Maps] add initial location option that fits to data bounds (elastic#74583) theme function (elastic#73451) [data.ui.query] Write filters to query log from default editor. (elastic#74474) [data.search.SearchSource] Move some SearchSource dependencies to the server. (elastic#74607) [Canvas][tech-debt] Convert renderers (elastic#74134) [security solutions][lists] Adds end to end tests (elastic#74473) pluralized for occurrences vs occurrence (elastic#74564) Update links that pointed to CONTRIBUTING.md (elastic#74757) [Ingest pipelines] Implement tabs in processor flyout (elastic#74469) [Event log] Use Alerts client & Actions client when fetching these types of SOs (elastic#73257) Bump chalk to 4.1.0 (elastic#73397) Index pattern field list - transition away from extending array - introduce and use getAll() (elastic#74718) [SECURITY] Bugs css/inspect (elastic#74711) [telemetry] update README to downplay ui_metrics (elastic#74635) Fixed grammar (elastic#74725) [Telemetry][API Integration] size_in_bytes to be a number (elastic#74664) [ILM] Convert node details flyout to TS (elastic#73707) [Ingest Node Pipelines] Sentence-case processor names (elastic#74645) ...
…71829) (#74782) * [Lens] Add styling options for x axis on the settings popover * ts related changes * Changes to the popover's design and y-axis implementatin * fix types and add unit tests * Add extra translations * Fix functional test and change the logic of the yTitle * fixes * fix showTitle settings bug * Fix ticklabels bug on y axes * fix some tests * Change the user flow on x and y titles on settings popover and enable the gridlines by default * disable linter warning * PR Comments * Add a comment to callback to explain the decision to listen only to open changes Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
…nes/processor-forms-a-d * 'master' of github.com:elastic/kibana: (25 commits) [ML] Removing full lodash library imports (elastic#74742) [Search] Server strategy example (elastic#71679) [Reporting] Fix and test for Listing of Reports (elastic#74453) [maps] fix drawing shapes (elastic#74689) [Resolver] Improve simulator. Add more click-through tests and panel tests. (elastic#74601) Deprecate schema-less specs in Vega (elastic#73805) [Security Solution] Rename Administration > Hosts subtab to Endpoints (elastic#74287) Timelion deprecation doc (elastic#74508) [Functional Tests] Adds a wait time between setting the index pattern and the time field on TSVB (elastic#74736) [Lens] Add styling options for x and y axes on the settings popover (elastic#71829) [Maps] add initial location option that fits to data bounds (elastic#74583) theme function (elastic#73451) [data.ui.query] Write filters to query log from default editor. (elastic#74474) [data.search.SearchSource] Move some SearchSource dependencies to the server. (elastic#74607) [Canvas][tech-debt] Convert renderers (elastic#74134) [security solutions][lists] Adds end to end tests (elastic#74473) pluralized for occurrences vs occurrence (elastic#74564) Update links that pointed to CONTRIBUTING.md (elastic#74757) [Ingest pipelines] Implement tabs in processor flyout (elastic#74469) [Event log] Use Alerts client & Actions client when fetching these types of SOs (elastic#73257) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/field_components/text_editor.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/manage_processor_form.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/append.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/bytes.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/circle.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/common_fields/field_name_field.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/common_fields/ignore_missing_field.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/convert.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/csv.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/date.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/date_index_name.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/dissect.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/dot_expander.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/drop.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/index.ts # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/shared.ts
Summary
Closes #68314 and #68313. Adds styling options for both x and y axes.
This PR adds more settings to the settings popover.
By default, the tick labels for x and y axes are enabled
while the gridlines ones are disabled.Update : We decided to go with the gridlines enabled by default
User can also hide the titles of x and y axes by switching off the corresponding switch. If the switch is on, then the user can also overwrite the title of each axis.
Note: I assume that if the user has both left and right y axes, then the y-axis title setting overwrites both. For this reason, only one y-axis title appears on the screenshot above. If we want to give the possibility to the user to change both titles then it needs an extra setting and makes things a little bit more complicated.
Checklist
Delete any items that are not applicable to this PR.
What to test
....