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.

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

Create a tree split widget #3969

Merged
merged 70 commits into from Oct 4, 2023
Merged

Create a tree split widget #3969

merged 70 commits into from Oct 4, 2023

Conversation

CarolineDenis
Copy link
Contributor

Fixes #3600

Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

looking good for the most part. I will do a deeper review when you say it's ready

specifyweb/frontend/js_src/lib/components/TreeView/Row.tsx Outdated Show resolved Hide resolved
specifyweb/frontend/js_src/lib/components/TreeView/Row.tsx Outdated Show resolved Hide resolved
specifyweb/frontend/js_src/lib/components/TreeView/Row.tsx Outdated Show resolved Hide resolved
specifyweb/frontend/js_src/lib/components/TreeView/Row.tsx Outdated Show resolved Hide resolved
specifyweb/frontend/js_src/lib/components/TreeView/Row.tsx Outdated Show resolved Hide resolved
@CarolineDenis
Copy link
Contributor Author

Split-scroll-open.mov
Edit_1.mov
Edit2.mov

Triggered by 5f28e46 on branch refs/heads/issue-3600
@realVinayak realVinayak requested a review from a team September 29, 2023 20:08
Copy link
Contributor

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

all good!

Copy link
Contributor

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

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

On narrow screens, the slider is made incredibly small

Screen.Recording.2023-09-29.at.10.31.21.PM.mov

@maxpatiiuk
Copy link
Member

should it fallback to horizontal on narrow screens? or hide the split view?

@grantfitzsimmons
Copy link
Contributor

grantfitzsimmons commented Sep 30, 2023

@maxpatiiuk Split view should probably not be supported on narrow screens. Horizontal would be fine!

@CarolineDenis
Copy link
Contributor Author

CarolineDenis commented Oct 2, 2023

@maxpatiiuk , do you have a better solution than the magic number '670' for narrow screen checker to disable split option?

@CarolineDenis
Copy link
Contributor Author

Screen.Recording.2023-10-02.at.8.59.26.AM.mov
Screen.Recording.2023-10-02.at.8.58.55.AM.mov

Triggered by d3ba552 on branch refs/heads/issue-3600
@maxpatiiuk
Copy link
Member

@CarolineDenis (optional) you can define the threathhold in rem instead of px if you want

( in specifyweb/frontend/js_src/lib/components/Atoms/index.tsx there is a variable called oneRem which corresponds to 1rem in CSS)

though the value of 640px is good too since it matches the threthhold value in tailwind

Copy link
Contributor

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

The changes you made look good, but I noticed one new problem (sorry for being late):

Your useEffect listen for screen width change, and in response to that the canSplit variable may get changed.

The problem is that the screen width may change a lot (imagine you are resizing the window), thus leading to the screenWidth state changing a lot, thus leading to a lot of re-renders, thus bad performance (or at least inefficient). And yet, it may be that despite all those changes to screenWith, the value of canSplit will still remain the same, thus all of those re-renders were useless.

So to optimize your code, instead of having screenWidth state, have a canSplit state, and set that state to true or false inside of useEffect in response to resize

Basically, the logic is the same, but by having canSplit be your state instead of screenWidth, you avoid all the useless re-renders of the component, as the component would only get updated on screen change only when canSplit changes

@CarolineDenis CarolineDenis merged commit c429ecd into production Oct 4, 2023
9 checks passed
@CarolineDenis CarolineDenis deleted the issue-3600 branch October 4, 2023 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Split Tree View
5 participants