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.

Sign up for GitHub

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

Jump to bottom

[internal] improve react code in UI mode, trace viewer and html report #31863

Closed
3 of 4 tasks
dgozman opened this issue Jul 25, 2024 · 3 comments
Closed
3 of 4 tasks

[internal] improve react code in UI mode, trace viewer and html report #31863

dgozman opened this issue Jul 25, 2024 · 3 comments
Assignees
@Skn0tt
Labels
v1.47

Comments

@dgozman
Copy link
Contributor

dgozman commented Jul 25, 2024 •
edited by Skn0tt
Loading

Some ideas:

  • introduce something like clsx for easier class names;
  • use react context to avoid passing many constant props around;
  • improve settings binding between localStorage and useState();
  • ...
@dgozman dgozman added the v1.47 label Jul 25, 2024
@dgozman dgozman mentioned this issue Jul 25, 2024
Merged
@mxschmitt
Copy link
Member

Would be probably also great to migrate away from ReactDOM.render to createRoot - since the render method is deprecated.

This was referenced Jul 29, 2024
Merged
Merged
Skn0tt added a commit that referenced this issue Jul 30, 2024
@Skn0tt
refactor(ui): synchronize settings via useSyncExternalStore instead o…
b8b5628 
…f prop drilling (#31911)

Broken out from #31900, part
of #31863.

Synchronizes different `useSettings` calls via `useSyncExternalStore`.
This saves us from having to drill down settings props everywhere,
without the big refactoring that a `Context` would be.
Skn0tt added a commit that referenced this issue Jul 31, 2024
@Skn0tt
refactor(ui): some react refactorings (#31900)
99724d0 
Addresses #31863. This PR
is chonky, but the individual commits should be easy to review. If
they're not, i'm happy to break them out into individual PRs.

There's two main things this does:

1. Remove some unused imports
2. Add a `clsx`-inspired helper function for classname templating

I wasn't able to replace `ReactDOM.render` with `ReactDOM.createRoot`.
This is the new recommended way starting with React 18, and the existing
one is going to be deprecated at some point. But it somehow breaks our
tests, i'll have to investigate that separately.
This was referenced Aug 8, 2024
Merged
Merged
Skn0tt added a commit that referenced this issue Aug 12, 2024
@Skn0tt
chore(recorder): update to React 18 (#32101)
45c2e8a 
Part of #31863. Updates
`recorder` to use React 18.
Skn0tt added a commit that referenced this issue Aug 12, 2024
@Skn0tt @mxschmitt
chore(ui): update to react 18 (#32079)
c8cc4f9 
Part of #31863. Updates
most of our React usage to React 18. `recorder` doesn't seem to like it
yet. I suspect that some of our code isn't compatible with concurrent
mode, i've investigated that in
#32101.

---------

Signed-off-by: Simon Knott <[email protected]>
Co-authored-by: Max Schmitt <[email protected]>
@Skn0tt
Copy link
Member

Skn0tt commented Aug 13, 2024

We've refactored some of the React code, introduced clsx, and updated all our React properties to use concurrent rendering. Considering this done!

@Skn0tt Skn0tt closed this as completed Aug 13, 2024
@mxschmitt
Copy link
Member

Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Skn0tt Skn0tt

Labels
v1.47
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dgozman @Skn0tt @mxschmitt