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

CSS: Animated Resizing of .panelContents when Window Size is less than 768 pixels. #344

Merged
merged 20 commits into from
Jan 19, 2024

Conversation

cmhhelgeson
Copy link
Contributor

Hello.Triangle.MSAA.-.WebGPU.Samples.-.Google.Chrome.2024-01-03.16-40-18.mp4

In order for this animation to be performant, I replaced the React state update with direct updates to the DOM based on window resizing and clicks, which prevents the state-triggered re-render that would stall the animation/transition. Since I'm unsure whether updating the DOM in this manner would be considered a "brittle" or "unidiomatic" practice, and since it modifies a longstanding component of this repository's code, I've tried to do my best to make the change as simple and unobtrusive as possible, as well as adding comments to the new window resize listener and stylePanel functions.

Merge main into my fork
Merge main into my fork.
…an 768px. React state updates were making this animation unperformant on all but the least intensive webgpu samples 'which was basically only helloTriangle'. Accordingly, I reworked the panelContents portion of the _app component, with css changes only occurring on window size changes or when toggling the expand button
…r reflect the element/ref they are directly modifying
@austinEng
Copy link
Collaborator

indeed it is risky to muck directly with the DOM because it could interfere with React. Would a pure CSS solution be possible?

haven't tested it, but perhaps something like:

.panel .panelContents {
   display: block;
   transition: max-height 0.25s ease-in;
 }
 
.panel[data-expanded=false] .panelContents {
    max-height: 0vh;
}

.panel[data-expanded=true] .panelContents {
  max-height: 100vh;
}

@cmhhelgeson
Copy link
Contributor Author

indeed it is risky to muck directly with the DOM because it could interfere with React. Would a pure CSS solution be possible?

haven't tested it, but perhaps something like:

.panel .panelContents {
   display: block;
   transition: max-height 0.25s ease-in;
 }
 
.panel[data-expanded=false] .panelContents {
    max-height: 0vh;
}

.panel[data-expanded=true] .panelContents {
  max-height: 100vh;
}

This was similar to a solution I tried earlier, using the current data-expanded state object to trigger the transition. However, the issue with that approach was that the state change seemed to trigger a re-render that made the animation imperceptible. I do agree with you that most of this can probably be changed to css, perhaps with the vanilla Javascript only containing a call to setAttribute for data-expanded rather than relying on React state. I'll work on that.

…l solution. All that's really needed to make this performant is a few css changes and changing data-Expanded from a React state variable that triggers a re-render to an attribute on a ref
@cmhhelgeson
Copy link
Contributor Author

Should be good now, I cut down the Javascript to just setting the attribute on the panel ref.

@cmhhelgeson
Copy link
Contributor Author

cmhhelgeson commented Jan 9, 2024

Just to reiterate, the impetus behind using setAttribute instead rather than tying data-expanded to a React state variable is to prevent a component re-render that stalls the more intensive samples for several seconds. You can actually see this problem on the current version of the website when clicking the expand button on the cornell sample.

@austinEng
Copy link
Collaborator

Sorry for taking a while to look at this again.

I don't feel comfortable mixing the DOM attribute manipulations while also using React here. Not an expert on this at all, but I think React provides facilities to say that things should not rerender. I believe React.memo helps with this.

@cmhhelgeson
Copy link
Contributor Author

Sorry for taking a while to look at this again.

I don't feel comfortable mixing the DOM attribute manipulations while also using React here. Not an expert on this at all, but I think React provides facilities to say that things should not rerender. I believe React.memo helps with this.

Not sure if this is good practice, but I figured memoizing the component was the easiest way to fix the issue.

Copy link
Collaborator

@austinEng austinEng left a comment

Choose a reason for hiding this comment

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

Nice! I think it's great. I verified with the React devtools profiler that we're not seeing any rerenders of that Component now

@cmhhelgeson
Copy link
Contributor Author

Nice! I think it's great. I verified with the React devtools profiler that we're not seeing any rerenders of that Component now

Glad to hear, think I just overcomplicated it earlier😅, but I think it's a good visual/usability fix.

@austinEng austinEng merged commit 36bce20 into webgpu:main Jan 19, 2024
1 check passed
@cmhhelgeson cmhhelgeson deleted the cmh_general_css branch January 19, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants