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

refactor RowHeightSettings component to EUI layout #203606

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from
Draft

refactor RowHeightSettings component to EUI layout #203606

wants to merge 20 commits into from

Conversation

akowalska622
Copy link
Contributor

@akowalska622 akowalska622 commented Dec 10, 2024 •
edited
Loading

Summary

Closes: [OneDiscover][UnifiedDataTable] Change RowHeightSettings design to align with EUI #203378

PR focuses on adjusting current RowHeightSettings view to EUI layout.
Current view:
image

EUI view:
image

Changes:

  1. Remove "Single" option
  2. Switch range slider to number input to control row height
    2.1) Switching between "Auto" and "Custom" options should persevere input value (but shouldn't influence row count if in "Auto" mode)
    2.2) While in "Auto" mode, number input should be disabled (but it shouldn't be hidden, like slider in current version)
    2.3) Switching back from "Auto" to "Custom" view, the number input value should be applied as row count
  3. "Auto-fit" copy should be changed to "Auto"
  4. Labels should be adjusted to new version

Keeping input state separate from row height settings, but in sync while custom mode is on, required some wider changes, both in Discover and Visualization. I also unified map of values for single and auto.
Previously Discover used -1 for auto and 0 for single, Visualization used undefined for auto and 1 for single.
Currently it's unified to -1 for auto and 1 for single. Please see table below.

Mode Discover (Previous) Visualization (Previous) Unified (Current)
auto -1 undefined -1
single 0 1 1

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

@akowalska622 akowalska622 added Feature:Discover Discover Application release_note:feature Makes this part of the condensed release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Dec 10, 2024
@akowalska622 akowalska622 self-assigned this Dec 10, 2024
@elasticmachine
Copy link
Contributor

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

  • Click to trigger kibana-pull-request for this PR!
  • Click to trigger kibana-deploy-project-from-pr for this PR!

@akowalska622 akowalska622 force-pushed the 203378-change-rowheightsettings-design-to-align-with-eui branch from 30c3dee to fbe834c Compare December 13, 2024 11:36
akowalska622
akowalska622 commented Dec 13, 2024
View reviewed changes
x-pack/plugins/lens/public/visualizations/datatable/components/toolbar.tsx Outdated
Copy link
Contributor Author

@akowalska622 akowalska622 Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The local state here can be refactored to a hook similar to use_row_height.ts in Discover - it would reduce the complexity of the joint state here and conditional updating depending on body cell height or header cell height. Maybe a good enhancement for Vis team 😁 For the sake of this task I didn't want to mess too much in your underlying state

@akowalska622 akowalska622 changed the title refactor RowHEightSettings component to EUI layout refactor RowHeightSettings component to EUI layout Dec 13, 2024
@akowalska622
Copy link
Contributor Author

/ci

1 similar comment
@akowalska622
Copy link
Contributor Author

/ci

@akowalska622 akowalska622 force-pushed the 203378-change-rowheightsettings-design-to-align-with-eui branch from cc2ecfb to 4b2c7e8 Compare January 7, 2025 13:09
@akowalska622
Copy link
Contributor Author

/ci

@akowalska622 akowalska622 force-pushed the 203378-change-rowheightsettings-design-to-align-with-eui branch from 9c3942c to dce9faa Compare January 8, 2025 11:18
@akowalska622 akowalska622 force-pushed the 203378-change-rowheightsettings-design-to-align-with-eui branch from c85d776 to 67c8bfc Compare January 8, 2025 11:47
@akowalska622
Copy link
Contributor Author

/ci

@elasticmachine
Copy link
Contributor

elasticmachine commented Jan 8, 2025 •
edited
Loading

⏳ Build in-progress, with failures

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #4 / discover/context_awareness extension getDefaultAppState ES|QL mode should render default state
  • [job] [logs] FTR Configs #8 / discover/context_awareness extension getDefaultAppState ES|QL mode should render default state
  • [job] [logs] FTR Configs #42 / discover/context_awareness extension getDefaultAppState ES|QL mode should render default state
  • [job] [logs] FTR Configs #45 / discover/context_awareness extension getDefaultAppState ES|QL mode should render default state
  • [job] [logs] FTR Configs #45 / discover/context_awareness extension getDefaultAppState ES|QL mode should render default state
  • [job] [logs] FTR Configs #4 / discover/context_awareness extension getDefaultAppState ES|QL mode should render default state
  • [job] [logs] FTR Configs #42 / discover/context_awareness extension getDefaultAppState ES|QL mode should render default state
  • [job] [logs] FTR Configs #8 / discover/context_awareness extension getDefaultAppState ES|QL mode should render default state
  • [job] [logs] FTR Configs #83 / discover/group2/data_grid2 discover data grid new line support should show new lines for "message" column except for Single row height setting
  • [job] [logs] FTR Configs #83 / discover/group2/data_grid2 discover data grid new line support should show new lines for "message" column except for Single row height setting
  • [job] [logs] FTR Configs #55 / discover/group2/data_grid3 discover data grid row height should allow to change row height
  • [job] [logs] FTR Configs #55 / discover/group2/data_grid3 discover data grid row height should allow to change row height

History

cc @akowalska622

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@akowalska622 akowalska622

Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Discover Discover Application release_note:feature Makes this part of the condensed release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[OneDiscover][UnifiedDataTable] Change RowHeightSettings design to align with EUI
2 participants
@akowalska622 @elasticmachine