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

BUGFIX: Render RightSideBar components even if the sidebar is hidden #3537

Conversation

grebaldi
Copy link
Contributor

fixes: #1744

The problem

Peek.2023-06-24.09-53.1744.before.mp4

In #1744 @maltemuth figured out that the rendering of the RightSideBarComponents is prevented in case the side bar is hidden.

Since the <Tabs/> component from @neos-project/react-ui-components is responsible for tracking the open-tab-state, removing the component from the rendering tree inevitably leads to a reset of that state.

The solution

Peek.2023-06-24.09-50.1744.after.mp4

I removed the condition, so that the RightSideBarComponents are now rendered regardless of the toggle-state of the side bar.

Since this is technically a removal of code that was intended to be a performance optimization, I should probably address the potential performance impact:

I believe the idea was to save on the creation of React elements in case they're not actually visible to the user. While this sounds intuitively plausible, I would theorize that this actually hurts performance. Preventing the React elements from being created will cause ReactDOM to remove the DOM tree represented by those elements. When the side bar becomes visible again, the React elements will again be created, which leads ReactDOM to create an entirely new DOM tree from those elements.

If the creation of the React elements isn't prevented, ReactDOM probably won't do anything at all. It would therefore be a fair assumption that this change will lead to a slight performance improvement :)

@github-actions github-actions bot added Bug Label to mark the change as bugfix 7.3 labels Jun 24, 2023
@grebaldi grebaldi linked an issue Jun 24, 2023 that may be closed by this pull request
Copy link
Member

@crydotsnake crydotsnake left a comment

Choose a reason for hiding this comment

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

Fine by reading

@crydotsnake crydotsnake requested a review from mhsdesign June 27, 2023 09:36
Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my unasked question about the performance aspect. You can read my mind.

@mhsdesign mhsdesign merged commit 99ab0e9 into neos:7.3 Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7.3 Bug Label to mark the change as bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remember open tab when hiding Inspector
3 participants