Closed JunmengXu closed 8 months ago
Absolutely, I will address the lint errors and also ensure that I adhere to the missing elements mentioned in the developer documentation. Thank you!
This PR accomplishes adding support for displaying hierarchical data to the vertical and horizontal headers of the matrix app, in other words, displaying the treeview.
This pull request introduces the following changes:
x_values_tree.json
and y_values_tree.json
as the hierarchical data given to the treeview components in the matrix app. Some random test data has been added to these two files.matrix-config
to support configuring xTree and yTree data independently.matrix
in the hooks
folder so that it will process the node data of the tree according to whether it provides xTree or yTree data, synchronize the display order of the data in the header and grid, and return the corresponding data according to whether xTree or yTree data is provided in the configuration file matrix-config.
matrix
in the pages
folder to support the search function for treeview and switching components between treeview and flat one independently.column-tree-headers
and row-tree-headers
components to show the horizontal and vertical tree views. These two components are similar. Each component contains provisions for the style of its constituent parts (text parts, icons, lines, indentation, highlighting) and logic for handling tree expansion nodes.grid-tree-cell
component, unlike grid-cell,
will display only the grid rows or columns corresponding to the nodes seen by the tree by judgment. The logic for highlighting cells changes dynamically as the treeview and flat components are switched.virtualized-tree-grid
component, which controls whether it independently shows the treeview or flat component for both row and column headers. It will also handle data synchronization, scroll, and search functions between the grid and header components.column-headers,
row-headers,
and virtualized-grid
components for hoveredCol and hoveredRow parts because it uses id instead of index for treeview.Known Issues:
React Hook useEffect in column-tree-headers
and row-tree-headers
with expanded and searchedColIDs has missing dependency, but adding all of the dependencies results in unnecessary extra refreshes. This may need more suggestions.
Thank you for your review and valuable feedback. I hope this summary helps in understanding the changes made in the PR and assists with its review. If there are any specific areas you'd like me to provide more details on, please let me know, and I'll be happy to do so.
We had a conversation about the changes and the following are the changes that we should focus on:
We still need to work more on performance. I suggest installing react-dev-tools chrome extension. It will allow you to see when components are getting rerendered. You can see that, as we assumed, we're rerendering both trees on each hover change. Before trying to change the implementation to avoid that, we can do other smaller performance improvements:
SVGIcon
component, let's use font-awesome icons. You can use <i className='fa-regular fa-square-minus'></i>
and <i className='fa-regular fa-square-plus'></i>
. I could feel the performance improvement after this tiny change.PlusSquare
, MinusSquare
, and CloseSquare
helps or not.For the highlight alignment issue, we're adding empty cells and headers in the current implementation. But Mui is ignoring the empty tree elements.
So you have to add an element after trees manually. For row headers, you can just add <div style={{height: cellHeight + 15}}></div>
, and that should solve the alignment. Solving the column headers
should be similar but would require more thinking because of the transform
that you've done.
In the current implementation, it doesn't matter if users scroll the headers or cells; the behavior is the same. That's why we're hiding the header scrollbars. But in this PR, we want users to be able to scroll the headers so the labels are visible. That's why we cannot just hide scrollbars. For column headers, we should show the vertical scrollbar. And for rows, we should show the horizontal scrollbar.
Using CSS we cannot specifically hide the horizontal scrollbar and show the vertical. So instead, we should just hide the scrolls with CSS all the time. And then, add a dummy element that has the scrollbar. We've done something similar in chaise apps. You can see how here we've added an element on top of the table. And then this function will make sure scrolling works.
Also, scrolling the header on Firefox and Safari is laggy. So you should take a look and see what's causing it.
The latest commit has effectively addressed the following issues:
margin: auto
' in line 70 of _matrix.scss
._matrix.scss
.<div style={{height: cellHeight + 15}}></div>
to row headers. Additionally, applied paddingBottom: cellWidth + 15
to the treeview style in column headers.Enhanced scrolling fluidity for row and column headers.
<i className='fa-regular fa-square-minus'></i>
and <i className='fa-regular fa-square-plus'></i>
.renderTree
, PlusSquare
, MinusSquare
, and CloseSquare
components.useState
to efficiently manage data with larger sizes.Implemented optimization to ensure smooth display of hovered entries. Modified updateRowId
and updateColId
to trigger only when headers are not being scrolled, akin to the default behavior of flat headers.
scrollTreeY
) reset to 0
upon page refresh. This led to the horizontal treeview scrolling to the top, displaying a blank section. This anomaly was unique to Firefox, occurring only during a page refresh, unlike Safari and Chrome. The issue was mitigated by utilizing a state to identify the first page load, subsequently ensuring that the horizontal treeview y position correctly aligns with the bottom.The latest commit made the following changes about Scroll behavior:
_matrix.scss
to move down the position of legend-container
grid-button.tsx
to move right the position of GridRightButton
, and move down the position of GridDownButton
grid-scrollbar.tsx
to enclose the scrollbar binding functions and componentsvirtualized-tree-grid.tsx
to add initialization functions for the dummy scrollbar and add the components into the parent componentKnown Issues: The current scrollbar applies to treeview components, while for flat components, the dummy scrollbar will still be displayed but has no real functionality as the scroll function has not yet been set. This problem will be solved during the refactoring process.
The latest commit made the following changes about refactor:
grid-tree-cell.tsx
and merged the code about treeview into the original flat component one, which is grid-cell.tsx
virtualized-tree-grid.tsx
and merged the code about treeview into the original flat component one, which is virtualized-grid.tsx
shared-row-headers.tsx
to extract the shared properties for row headers of both flat and treeview componentsshared-column-headers.tsx
to extract the shared properties for column headers of both flat and treeview componentscolumn-headers.tsx
and column-tree-headers.tsx
to inherit shared-column-headers.tsx
row-headers.tsx
and row-tree-headers.tsx
to inherit shared-row-headers.tsx
matrix.tsx
in pages
, matrix.ts
in hooks
, virtualized-grid.tsx
, row-headers.tsx
, column-headers.tsx
and grid-cell.tsx
to unify the key as item ID for hover and search function instead of indexThe latest commit focus on the following issues:
src/utils/ui-utils.ts
_matrix.scss
/***/
syntax for function commentsreturn
for 'if' statementsmatrix_config.js
, models/matrix-config.ts
to add new configurations for rowHeader
and columnHeader
pages/matrix.tsx
to handle the contents and default setting of the new configurations virtualized-grid.tsx
, shared-row-headers.tsx
, row-headers.tsx
, and row-tree-headers
to add a new container outside the header and pass the parameters about scroll.grid-scrollbar.tsx
file still exists, which should be removed.The latest two commits focus on the following issues:
shared-row-headers.tsx
to add a grid-row-headers-container
to set the width of the row header, and add grid-row-headers-horizontal-scroll
to set the max scrollable width and handle the horizontal scroll function.row-headers.tsx
to remove redundant elements and logic of the horizontal scroll function.row-tree-headers.tsx
to remove redundant elements and logic of the horizontal scroll function, and add new function and change parameters to handle the hover effect and dynamically adjust the hover effect position which effected by horizontal scroll.shared-column-headers.tsx
to add a grid-column-headers-container
to set the height of the column header, and add grid-column-headers-vertical-scroll
to set the max scrollable height and handle the vertical scroll function. Also, control the initial position of the scrollbar.column-headers.tsx
to remove redundant elements and logic of the vertical scroll function.column-tree-headers.tsx
to remove redundant elements and logic of the vertical scroll function, and add new function and change parameters to handle the hover effect and dynamically adjust the hover effect position which effected by vertical scroll. Since the column tree header involves transformation, the div
style is adjusted dynamically when we set the scrollableMaxHeight
as auto
. virtualized-grid.tsx
to control the initialization of the dummy scrollbar for both row and column headers_matrix.scss
to add grid-row-headers-container
and grid-column-headers-container
style.ui-utils.ts
to handle updated binding logic of dom elements and adjust the width or height of topScrollElement
dynamically when the scrollableMaxWidth
or scrollableMaxHeight
is auto
. Initialize the scrollbar position of topScrollElementWrapper
to the very bottom since in default it will be at the very top and not show the header content correctly.auto
. The component width or height is calculated by font size and characters.scrollableMaxWidth
is auto, the hover effect for each entry not in the top layer leaves some blank at the very left since the indent of the original MUI tree component.grid-scrollbar.tsx
file still exists, which should be removed. (Will be handled at next commit)virtualized-grid.tsx
. (Will be handled at the next commit, once the scroll function of current version meets the requirement)The latest commit focuses on the following issues:
grid-scrollbar.tsx
utils/config.ts
with comments, which is used to parse and manipulate the headers configurations.ui-utils.ts
, added comment explaining the dummy scrollbar functions, pointed the actual code lines that are different from Chaise
, removed the console.logvirtualized-grid.tsx
, removed code and logic of horizontal scrolling for yTree and vertical scrolling for xTree, since this logic has been moved to shared headers.virtualized-grid.tsx
, refactored data filter function about the visiable rows and columns in GridDatatree.ts
under src/utils
, moved two functions checkParentChainExist and getParentChain there, which were identical originally between column-tree-headers.tsx
and row-tree-headers.tsx
. tree-button.tsx
file under src/components
, moved three icon components and the tree render component there, and refactored these functions. These components were similar originally in the row and column headers.The latest commit focuses on the following issues:
Modified grid-button.tsx
, pass and use columnHeaderHeight
for the height of GridLeftButton
and GridRightButton
, which is similar to how the GridUpButton
and GridDownButton
's width is rowHeaderWidth
. This will make sure the icon is vertically aligned in the middle.
During the test, found an edge case UI issue. This only happened when we set the column header's scrollable max height as auto, the column is a tree header, and the column header height is longer than the height of the inner div grid-column-headers
. This results in the inner div dom appearing in the middle of the whole column header area, which means it is not close to the grid but leaves a big blank area between the two.
To address it:
Modified column-tree-headers.tsx
, pass the height of the div element grid-column-headers
to the shared component.
Modified shared-column-headers.tsx
, got the parameter, and used it to calculate the 'bottom' position style for div element grid-column-headers-horizontal-scroll
Complete the development of treeview component, support users to switch the x and y axis of the tree component and flat component independently in the configuration file, the style of the tree component will change dynamically with the configuration file.
The changes are mainly done by creating different React components, and modifying a small amount of code in the original tsx file.