🐛(frontend) show full nested doc names with resizable panel#1456
Conversation
0abe61e to
f0c1194
Compare
3e294e9 to
c255024
Compare
c255024 to
3406fee
Compare
|
Size Change: +11.2 kB (+0.31%) Total Size: 3.67 MB
|
3406fee to
f059014
Compare
AntoLC
left a comment
There was a problem hiding this comment.
The feature works great, I think we can improve the code though, by separating more the components in MainLayout.
I can see we can move the left panel on the grid page as well, I thought it was only with the tree part, is it as expected ?
There was a problem hiding this comment.
The yarn.lock gives changes in local.
| display: ${menuOpen || !isDesktop ? 'flex' : 'none'}; | ||
| position: absolute; | ||
| display: flex; | ||
| position: sticky; | ||
| right: 0; | ||
|
|
||
| opacity: ${menuOpen || !isDesktop ? '1' : '0'}; |
There was a problem hiding this comment.
By doing like that the name is cropped far before needed, because the actions button take place even when not displayed.
There was a problem hiding this comment.
Not introduce in this PR but I can see there is differences on how the actions buttons are displayed between the top parent and the children, I think it should be &:focus-visible here.
| <PanelGroup | ||
| autoSaveId="docs-left-panel-persistence" | ||
| direction="horizontal" | ||
| > | ||
| <Panel | ||
| ref={ref} | ||
| order={0} | ||
| defaultSize={minPanelSize} | ||
| minSize={minPanelSize} | ||
| maxSize={maxPanelSize} | ||
| > | ||
| <LeftPanel /> | ||
| </Panel> | ||
| <PanelResizeHandle | ||
| className="border-clr-surface-primary" | ||
| style={{ | ||
| borderRightWidth: '1px', | ||
| borderRightStyle: 'solid', | ||
| borderRightColor: colorsTokens['greyscale-200'], | ||
| width: '1px', | ||
| cursor: 'col-resize', | ||
| }} | ||
| /> | ||
| <Panel order={1}> | ||
| <Box | ||
| as="main" | ||
| role="main" | ||
| aria-label={t('Main content')} | ||
| id={MAIN_LAYOUT_ID} | ||
| $align="center" | ||
| $width="100%" | ||
| $height={`calc(100dvh - ${HEADER_HEIGHT}px)`} | ||
| $padding={{ | ||
| all: 'base', | ||
| }} | ||
| $background={ | ||
| currentBackgroundColor === 'white' | ||
| ? colorsTokens['greyscale-000'] | ||
| : colorsTokens['greyscale-050'] | ||
| } | ||
| $css={css` | ||
| overflow-y: auto; | ||
| overflow-x: clip; | ||
| `} | ||
| > | ||
| {children} | ||
| </Box> | ||
| </Panel> | ||
| </PanelGroup> | ||
| ) : ( |
There was a problem hiding this comment.
It should have its own component I think.
| <Box | ||
| as="main" | ||
| role="main" | ||
| aria-label={t('Main content')} | ||
| id={MAIN_LAYOUT_ID} | ||
| $align="center" | ||
| $width="100%" | ||
| $height={`calc(100dvh - ${HEADER_HEIGHT}px)`} | ||
| $padding={{ | ||
| all: 'base', | ||
| }} | ||
| $background={ | ||
| currentBackgroundColor === 'white' | ||
| ? colorsTokens['greyscale-000'] | ||
| : colorsTokens['greyscale-050'] | ||
| } | ||
| $css={css` | ||
| overflow-y: auto; | ||
| overflow-x: clip; | ||
| `} | ||
| > | ||
| {children} | ||
| </Box> | ||
| </Panel> |
There was a problem hiding this comment.
I can see the code almost the same for the mobile part under, can we factorize this part.
| <LeftPanel /> | ||
| </Panel> | ||
| <PanelResizeHandle | ||
| className="border-clr-surface-primary" |
| useEffect(() => { | ||
| const handleResizeStart = () => { | ||
| setIsResizing(true); | ||
| if (resizeTimeoutRef.current) { | ||
| clearTimeout(resizeTimeoutRef.current); | ||
| } | ||
| resizeTimeoutRef.current = window.setTimeout(() => { | ||
| setIsResizing(false); | ||
| }, 150); | ||
| }; | ||
|
|
||
| window.addEventListener('resize', handleResizeStart); | ||
|
|
||
| return () => { | ||
| window.removeEventListener('resize', handleResizeStart); | ||
| if (resizeTimeoutRef.current) { | ||
| clearTimeout(resizeTimeoutRef.current); | ||
| } | ||
| }; | ||
| }, []); | ||
|
|
||
| // Keep pixel-based constraints while the library works in percentages. | ||
| // We translate px -> % on mount and on viewport resizes so that: | ||
| // - min stays ~300px, max stays ~450px (capped to 40% on small screens) | ||
| // - on mobile, the left panel collapses (min = 0) | ||
| // - when enableResize is false, we lock the size by setting max == min | ||
| useEffect(() => { | ||
| const updatePanelSize = () => { | ||
| const min = Math.round(calculateDefaultSize(MIN_PANEL_SIZE, isDesktop)); | ||
| const max = Math.round( | ||
| Math.min(calculateDefaultSize(MAX_PANEL_SIZE, isDesktop), 40), | ||
| ); | ||
| setMinPanelSize(isDesktop ? min : 0); | ||
| enableResize ? setMaxPanelSize(max) : setMaxPanelSize(min); | ||
| }; | ||
|
|
||
| updatePanelSize(); | ||
| window.addEventListener('resize', updatePanelSize); | ||
|
|
||
| return () => { | ||
| window.removeEventListener('resize', updatePanelSize); | ||
| }; | ||
| }, [isDesktop, enableResize]); |
There was a problem hiding this comment.
This part can be improve I think, we use twice the same listener (resize), we could use only one.
If isDesktop is false, I have the feeling we don't need to listen anything.
When using useEffect, it is good practice to use useCallback around the function called inside, calculateDefaultSize should be inside a useCallback.
I think by having its own component we will be able to remove this notion of isDesktop, this component will be called only when desktop, removing complexity.
6d49207 to
3326f02
Compare
| minSize={minPanelSize} | ||
| maxSize={maxPanelSize} | ||
| > | ||
| <LeftPanel /> |
There was a problem hiding this comment.
Could it be a parameter as well, to be fully generic ?
| "react-dom": "*", | ||
| "react-i18next": "15.7.3", | ||
| "react-intersection-observer": "9.16.0", | ||
| "react-resizable-panels": "^3.0.6", |
There was a problem hiding this comment.
| "react-resizable-panels": "^3.0.6", | |
| "react-resizable-panels": "3.0.6", |
src/frontend/yarn.lock
Outdated
There was a problem hiding this comment.
You should not remove the yarn.lock, you should put back the previous one, then do yarn, it will only include the new dependency react-resizable-panels and what is needed with it.
| > | ||
| {children} | ||
| </Box> | ||
| {renderContent()} |
There was a problem hiding this comment.
It is a bit anti-pattern like that, what about its own component MainLayoutContent ?
| {renderContent()} | |
| <MainLayoutContent enableResizablePanel={enableResizablePanel}>{children}</MainLayoutContent> |
CHANGELOG.md
Outdated
| - 🐛(frontend) reduce no access image size from 450 to 300 #1463 | ||
| - 🐛(frontend) preserve interlink style on drag-and-drop in editor #1460 | ||
| - ✨(frontend) load docs logo from public folder via url #1462 | ||
| - 🐛(frontend) show full nested doc names with ajustable bar #1456 |
There was a problem hiding this comment.
Under the Unreleased part.
41c4f3b to
4df1842
Compare
e83ad59 to
acb1d71
Compare
mainlayout and leftpanel updated with resizable panel saved in localstorage Signed-off-by: Cyril <[email protected]> ✨(frontend) show full nested doc names with horizontal scroll support horizontal overflow enabled and opacity used for sticky actions visibility Signed-off-by: Cyril <[email protected]> ✨(frontend) show full nested doc names with horizontal scroll support horizontal overflow enabled and opacity used for sticky actions visibility Signed-off-by: Cyril <[email protected]> ✨(frontend) add resizable-panels lib also used in our shared ui kit needed for adaptable ui consistent with our shared ui kit components Signed-off-by: Cyril <[email protected]>
08af57d to
a833fdc
Compare
Purpose
EDIT & FINAL: Following a discussion with @rl-83, we replaced the horizontal scroll approach with an adjustable panel bar using
react-resizable-panels.This change improves usability by allowing users to resize the document tree panel within defined limits. It ensures better visibility of nested documents without using scrollbars and aligns with the implementation used in our shared UI Kit (where resizing is handled via the
MainLayout, not inside the document tree component).added e2e tests to cover this new resizable behavior too.
issue : 1180
ajustbar.mp4
Proposal
react-resizable-panels(v3.0.6) dependencyPanelGroupwith a horizontal resize handle inMainLayoutlocalStorage