Discover page collapsible panels #5635

Ganesh0107 · 2023-12-21T19:30:08Z

Description

This PR resolves Discover 2.0's missing left sidebar expand/collapse features by integrating OUI's resizable container

Issues Resolved

Closes #4780 [Data Explorer] Left side bar can not be expanded and collapsed

Screenshot

Screen.Recording.2023-12-22.at.12.54.52.AM.mov

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

vvvprabhakar · 2023-12-21T19:31:37Z

Looks good

ashwin-pc

@Ganesh0107 Thanks for the PR! I have tested the change locally and I have a few comments on the PR:

The DCO check has failed. The project needs never commit in the PR to be signed off using git's signoff feature -s. Can you check the details for the DCO workflow failure for step on how to remedy it?
The EUI Resize component's children are rerndered every time a resize event happens. This causes performance issues. Can you memoize its children to prevent this. Here is how another application that uses the same component does it: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/vis_builder/public/application/components/workspace.tsx#L157-L159. This is not a blocker but will really improve the change :)

ashwin-pc · 2023-12-22T21:47:36Z

src/plugins/data_explorer/public/components/app_container.tsx

+          <EuiResizableContainer>
+            {(EuiResizablePanel, EuiResizableButton) => (
+              <>
+                <EuiResizablePanel initialSize={140} minSize="10%" mode="collapsible">


Suggested change

<EuiResizablePanel initialSize={140} minSize="10%" mode="collapsible">

<EuiResizablePanel initialSize={140} minSize="10%" mode="collapsible" paddingSize="none">

Nit: would be nice to give the left panel a bit right padding after setting paddingSize="none" , like style={{ paddingRight: 8 }}, otherwise the resize button is overlapping with the right border:

Thats fine for now. I'd leave minor modifications to spacing and padding to a full UX review since they have to match other experiences too.

Sounds good, I'm fine with that :)

Updated style={{ paddingRight: 8 }}

ashwin-pc · 2023-12-22T21:47:56Z

src/plugins/data_explorer/public/components/app_container.tsx

+                </EuiResizablePanel>
+                <EuiResizableButton />
+
+                <EuiResizablePanel initialSize={1140} minSize="65%" mode="main">


Suggested change

<EuiResizablePanel initialSize={1140} minSize="65%" mode="main">

<EuiResizablePanel initialSize={1140} minSize="65%" mode="main" paddingSize="none">

codecov · 2023-12-25T02:40:56Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (7c0bd9f) 67.01% compared to head (2c6b02b) 67.03%.

❗ Current head 2c6b02b differs from pull request most recent head 1d82737. Consider uploading reports for the commit 1d82737 to get more accurate results

Files	Patch %	Lines
.../data_explorer/public/components/app_container.tsx	50.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5635      +/-   ##
==========================================
+ Coverage   67.01%   67.03%   +0.02%     
==========================================
  Files        3296     3296              
  Lines       63370    63349      -21     
  Branches    10093    10089       -4     
==========================================
+ Hits        42465    42468       +3     
+ Misses      18456    18431      -25     
- Partials     2449     2450       +1

Flag	Coverage Δ
Linux_1	`35.23% <ø> (ø)`
Linux_2	`55.18% <ø> (+0.06%)`	⬆️
Linux_3	`43.92% <50.00%> (-0.02%)`	⬇️
Linux_4	`35.33% <ø> (ø)`
Windows_1	`35.26% <ø> (ø)`
Windows_2	`55.15% <ø> (+0.06%)`	⬆️
Windows_3	`43.93% <50.00%> (+<0.01%)`	⬆️
Windows_4	`35.33% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ruanyl · 2023-12-26T05:33:46Z

src/plugins/data_explorer/public/components/app_container.tsx

+          <EuiResizableContainer>
+            {(EuiResizablePanel, EuiResizableButton) => (
+              <>
+                <EuiResizablePanel initialSize={140} minSize="10%" mode="collapsible">


initialSize is number in percent according to OUI documentation, could you check if initialSize={140} and initialSize={1140} are correctly used?

Thanks @ruanyl , Updated the initial size with percent according to OUI documentation.

ruanyl · 2023-12-26T05:39:42Z

src/plugins/data_explorer/public/components/app_container.tsx

+          <EuiResizableContainer>
+            {(EuiResizablePanel, EuiResizableButton) => (
+              <>
+                <EuiResizablePanel initialSize={140} minSize="10%" mode="collapsible">


Nit: would be nice to give the left panel a bit right padding after setting paddingSize="none" , like style={{ paddingRight: 8 }}, otherwise the resize button is overlapping with the right border:

ruanyl · 2023-12-26T05:41:19Z

src/plugins/data_explorer/public/components/app_container.scss

-  min-width: 400px;
+  max-width: 620px;
+  min-width: 180px;
+  top: 0;


This top: 0 seems not necessary

ruanyl · 2023-12-26T05:48:03Z

Thanks for the improvement! Could you also add an entry about this change to CHANGELOG.md?

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

ashwin-pc · 2024-01-11T11:31:23Z

@Ganesh0107 will you able to make the changes suggested in the review? We are currently tracking this issue for the 2.12 release, so let me know if I can help in any way here :)

Ganesh0107 · 2024-01-11T12:36:52Z

@ashwin-pc I have made the changes and testing them locally, will commit the changes soon

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

AMoo-Miki · 2024-01-11T18:39:06Z

src/plugins/data_explorer/public/components/app_container.scss

+  max-width: 620px;
+  min-width: 180px;


I am not a fan of these. Wouldn't they limit how far I can narrow or expand the panels? Wouldn't having a min prevent me from totally collapsing the left-bar if i desired?

Also, shouldn't these be unnecessary, even unwanted, inside a resizable-container?

I am having trouble seeing the need for these boundaries.

We any way have buttons for collapse and expand, this will not prevent from completely closing it

onClick of the button , the side bar will be collapsed.

screen-recording-2024-01-12-at-122141-am_kPc2t5La.mp4

Can we eliminate these two styles. They dont seem necessary anymore

ashwin-pc · 2024-01-17T00:22:13Z

@Ganesh0107 Looks like your latest change has neither the paddingSize="none" prop or the padding right that @ruanyl suggested. Can you check and see if you have pushed all your changes?

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

Ganesh0107 · 2024-01-18T19:10:13Z

@ashwin-pc Apologies for the oversight. I've pushed the latest changes, including the missing CSS adjustment. Please review the updated code, and let me know if there are any further adjustments needed.

ashwin-pc

Thanks for the changes @Ganesh0107. I pulled down the changes and have a few more minor changes that are needed. The biggest one being that this layout has to render correctly in mobile too. Ive added all my suggestions inline so that its easy but once those are made the PR should be good to go :)

ashwin-pc · 2024-01-18T23:19:07Z

src/plugins/data_explorer/public/components/app_container.tsx

+                  initialSize={25}
+                  minSize="10%"
+                  mode="collapsible"
+                  style={{ paddingRight: 8 }}


Suggested change

style={{ paddingRight: 8 }}

style={isMobile ? { paddingBottom: 8 } : { paddingRight: 8 }}

paddingSize="none"

ashwin-pc · 2024-01-18T23:19:26Z

src/plugins/data_explorer/public/components/app_container.tsx

+                </EuiResizablePanel>
+                <EuiResizableButton />
+
+                <EuiResizablePanel initialSize={75} minSize="65%" mode="main">


Suggested change

<EuiResizablePanel initialSize={75} minSize="65%" mode="main">

<EuiResizablePanel initialSize={75} minSize="65%" mode="main" paddingSize="none">

ashwin-pc · 2024-01-18T23:33:59Z

src/plugins/data_explorer/public/components/app_container.scss

+  max-width: 620px;
+  min-width: 180px;


Can we eliminate these two styles. They dont seem necessary anymore

ashwin-pc · 2024-01-18T23:34:56Z

src/plugins/data_explorer/public/components/app_container.tsx

-          <EuiPageBody className="deLayout__canvas">
-            <Canvas {...params} />
-          </EuiPageBody>
+          <EuiResizableContainer>


Suggested change

<EuiResizableContainer>

<EuiResizableContainer direction={isMobile ? 'vertical' : 'horizontal'}>

And you can get the value of isMobile as follows:

import { ..., useIsWithinBreakpoints } from '@elastic/eui'; // ... const isMobile = useIsWithinBreakpoints(['xs', 's', 'm']);

ashwin-pc · 2024-01-18T23:36:48Z

src/plugins/data_explorer/public/components/app_container.tsx

+              <>
+                <EuiResizablePanel
+                  initialSize={25}
+                  minSize="10%"


Suggested change

minSize="10%"

minSize="260px"

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

Ganesh0107 · 2024-01-23T11:26:23Z

@ashwin-pc Updated the suggested changes, currently adding test cases. PR will be ready for review shortly. Let me know if further adjustments are required. Thanks!

ashwin-pc

Looks like you missed on of my suggestions. The rest looks good :)

src/plugins/data_explorer/public/components/app_container.tsx

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

Co-authored-by: Ashwin P Chandran <ashwinpc1993@gmail.com> Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

Ganesh0107 · 2024-01-25T07:34:33Z

@ashwin-pc Thanks for letting me know. I've made the necessary changes

ashwin-pc

@Ganesh0107 thanks for making the changes, the PR looks good. Sorry but i noticed one other change thats needed. The Changelog entry you have made is for the 1.x releases. Since this change has not been released yet, it should go to the unreleased section of the file, which is in and around line 30

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

This reverts commit 34a1b12.

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

ashwin-pc · 2024-02-06T23:37:35Z

@Ganesh0107 thanks for the PR. I have incorporated your changes in #5789 and will credit you for your contribution on that PR instead of this one since I had to make changes on top of yours for another feature. Closing this PR for now since the changes have already been incorporated elsewhere.

Ganesh0107 requested review from ananzh, kavilla, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, kristenTian, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, BSFishy, curq, bandinib-amzn and SuZhou-Joe as code owners December 21, 2023 19:30

github-actions bot added the first-time-contributor label Dec 21, 2023

Ganesh0107 mentioned this pull request Dec 21, 2023

[Data Explorer] Left side bar can not be expanded and collapsed #4780

Closed

ashwin-pc reviewed Dec 22, 2023

View reviewed changes

ruanyl reviewed Dec 26, 2023

View reviewed changes

Ganesh0107 added 3 commits December 26, 2023 18:21

Discover page Changes

2a18d8e

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

Discover page changes

bc9d9a4

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

Css Changes

a071290

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

Ganesh0107 force-pushed the DiscoverPage-CollapsiblePanels branch from 169db75 to a071290 Compare December 26, 2023 12:52

Merge branch 'main' into DiscoverPage-CollapsiblePanels

a9c6810

Discover page collapsible Nav PR Changes

d5e9193

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

AMoo-Miki mentioned this pull request Jan 11, 2024

[Discover] Narrow document details column #5683

Closed

InitalSize OUI Documentaion percentage Changes

0e603a9

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

Ganesh0107 requested review from ruanyl and ashwin-pc January 11, 2024 18:33

AMoo-Miki reviewed Jan 11, 2024

View reviewed changes

Collapsible panel button css change

f1b9ec2

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

ashwin-pc reviewed Jan 18, 2024

View reviewed changes

Collapsible panel- Responsive Changes

6f907dc

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

ananzh mentioned this pull request Jan 19, 2024

[Discover] column scaling regression in discover #5671

Open

Refactor sidebar style for mobile view

d5f9fa0

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

ashwin-pc reviewed Jan 25, 2024

View reviewed changes

src/plugins/data_explorer/public/components/app_container.tsx Show resolved Hide resolved

Ganesh0107 and others added 3 commits January 25, 2024 12:56

Add paddingSize=none to Sidebar component

51da248

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

Update src/plugins/data_explorer/public/components/app_container.tsx

79a38d8

Co-authored-by: Ashwin P Chandran <ashwinpc1993@gmail.com> Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

Add paddingSize=none to Sidebar component

2c6b02b

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

ashwin-pc reviewed Jan 25, 2024

View reviewed changes

ananzh added backport 2.x discover for discover reinvent v2.12.0 backport 2.11 labels Jan 26, 2024

Ganesh0107 added 4 commits February 3, 2024 22:44

Merge branch 'main' into DiscoverPage-CollapsiblePanels

5cd0b03

ChangeLog Update

34a1b12

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

Revert "ChangeLog Update"

e56e6ca

This reverts commit 34a1b12.

ChangeLog Update-unreleased section

1d82737

Signed-off-by: Ganesh0107 <ganesh.gopal@freshworks.com>

ashwin-pc closed this Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discover page collapsible panels #5635

Discover page collapsible panels #5635

Ganesh0107 commented Dec 21, 2023

vvvprabhakar commented Dec 21, 2023

ashwin-pc left a comment

ashwin-pc Dec 22, 2023

ruanyl Dec 26, 2023

ashwin-pc Jan 8, 2024

ruanyl Jan 9, 2024

Ganesh0107 Jan 11, 2024

ashwin-pc Dec 22, 2023

codecov bot commented Dec 25, 2023 •

edited

Loading

ruanyl Dec 26, 2023 •

edited

Loading

Ganesh0107 Jan 11, 2024

ruanyl Dec 26, 2023

ruanyl Dec 26, 2023

ruanyl commented Dec 26, 2023

ashwin-pc commented Jan 11, 2024

Ganesh0107 commented Jan 11, 2024

AMoo-Miki Jan 11, 2024 •

edited

Loading

vvvprabhakar Jan 11, 2024

Ganesh0107 Jan 11, 2024

ashwin-pc Jan 18, 2024

ashwin-pc commented Jan 17, 2024

Ganesh0107 commented Jan 18, 2024

ashwin-pc left a comment

ashwin-pc Jan 18, 2024

ashwin-pc Jan 18, 2024

ashwin-pc Jan 18, 2024

ashwin-pc Jan 18, 2024

ashwin-pc Jan 18, 2024

Ganesh0107 commented Jan 23, 2024

ashwin-pc left a comment

Ganesh0107 commented Jan 25, 2024

ashwin-pc left a comment

ashwin-pc commented Feb 6, 2024

	<EuiResizablePanel initialSize={140} minSize="10%" mode="collapsible">
	<EuiResizablePanel initialSize={140} minSize="10%" mode="collapsible" paddingSize="none">

	<EuiResizablePanel initialSize={1140} minSize="65%" mode="main">
	<EuiResizablePanel initialSize={1140} minSize="65%" mode="main" paddingSize="none">

	style={{ paddingRight: 8 }}
	style={isMobile ? { paddingBottom: 8 } : { paddingRight: 8 }}
	paddingSize="none"

	<EuiResizablePanel initialSize={75} minSize="65%" mode="main">
	<EuiResizablePanel initialSize={75} minSize="65%" mode="main" paddingSize="none">

	<EuiResizableContainer>
	<EuiResizableContainer direction={isMobile ? 'vertical' : 'horizontal'}>

Discover page collapsible panels #5635

Discover page collapsible panels #5635

Conversation

Ganesh0107 commented Dec 21, 2023

Description

Issues Resolved

Screenshot

Check List

vvvprabhakar commented Dec 21, 2023

ashwin-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 25, 2023 • edited Loading

Codecov Report

ruanyl Dec 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanyl commented Dec 26, 2023

ashwin-pc commented Jan 11, 2024

Ganesh0107 commented Jan 11, 2024

AMoo-Miki Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwin-pc commented Jan 17, 2024

Ganesh0107 commented Jan 18, 2024

ashwin-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ganesh0107 commented Jan 23, 2024

ashwin-pc left a comment

Choose a reason for hiding this comment

Ganesh0107 commented Jan 25, 2024

ashwin-pc left a comment

Choose a reason for hiding this comment

ashwin-pc commented Feb 6, 2024

codecov bot commented Dec 25, 2023 •

edited

Loading

ruanyl Dec 26, 2023 •

edited

Loading

AMoo-Miki Jan 11, 2024 •

edited

Loading