-
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
[ML] Add responsive layout to Index data visualizer, fix doc count chart margin #147137
Conversation
Pinging @elastic/ml-ui (:ml) |
@elasticmachine merge upstream |
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.
Hi @qn895, just a few notes:
Looks like the top section goes outside the page a bit as we go to lower breakpoints. This might be as simple as removing the min-width on the data view title—I think the breakpoint kicks in before that condenses too much.
I would also recommend putting in some margin between the data view button and the 'use full data' button. I'd suggest 16px to denote that these are different sections, and keep the 8px spacing between items in a section. Tiny nit, but it does clean it up.
Not sure how much we should concern ourselves with this small of a breakpoint, but as the table switches to the mobile view, seems like there's some oddities in the order of things. Perhaps something worth exploring separately depending on the need?
Should we consider truncating the name to avoid simply cutting off the text? Possible that this could look like a shorter field name that it actually is.
I think we could decrease the size of these icons to give the text more room. Again, a tiny nit and not a blocker, but would help clean this up a bit.
I will mark this as a blocker, but acknowledge not all of these are required and primarily suggestions. The first point is probably the highest priority IMO. But happy to discuss these further.
@peteharverson I agree and thanks for adding/catching these! |
..._visualizer/public/application/common/components/date_picker_wrapper/date_picker_wrapper.tsx
Show resolved
Hide resolved
This behavior isn't from your changes here, but wondering if we should change it as part of this PR. The 'Explore your data' title is tied to the visibility of the Discover card, and isn't shown if just the ML cards are shown. I'm thinking we should show this section title if any of the cards are visible: With Discover plus ML write access:Without Discover access: |
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.
Gave this another test and all the layout fixes look good. The only issue I'm still seeing is where the date picker doesn't update with 'Use full data' for users without access to Discover.
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.
Took another look and everything is looking good! Thanks for making those edits!
@elasticmachine merge upstream |
Some observations regarding the visual part:
|
..._visualizer/public/application/common/components/date_picker_wrapper/date_picker_wrapper.tsx
Outdated
Show resolved
Hide resolved
...ation/common/components/document_count_content/document_count_chart/document_count_chart.tsx
Outdated
Show resolved
Hide resolved
// temporary fix to reduce horizontal chart margin until fixed in Elastic Charts itself | ||
labelFormat={useLegacyTimeAxis ? undefined : () => ''} |
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.
could you please create an issue in the elastic-charts repo and mention the link in the comment?
...data_visualizer/public/application/common/components/field_count_panel/field_count_panel.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/data_visualizer/public/application/common/components/link_card/link_card.tsx
Outdated
Show resolved
Hide resolved
...n/index_data_visualizer/components/index_data_visualizer_view/index_data_visualizer_view.tsx
Outdated
Show resolved
Hide resolved
...n/index_data_visualizer/components/index_data_visualizer_view/index_data_visualizer_view.tsx
Outdated
Show resolved
Hide resolved
...n/index_data_visualizer/components/index_data_visualizer_view/index_data_visualizer_view.tsx
Outdated
Show resolved
Hide resolved
...lizer/public/application/index_data_visualizer/components/search_panel/field_type_filter.tsx
Outdated
Show resolved
Hide resolved
@@ -240,15 +242,36 @@ export const DataVisualizerUrlStateContextProvider: FC< | |||
[history, urlSearchString] | |||
); | |||
|
|||
const [panelWidth, setPanelWidth] = useState(1600); |
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.
do we actually need a default static value?
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.
Fix for the Use Full Data button looks good, and rest of the changes to fix the responsive layout LGTM
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
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @qn895 |
Summary
This PR addresses #137257 and better handles the layout when the screen width decreases. Changes include:
For reviewers:
.scss
files were deletedBefore
After

Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers