Scheduling Profiler: Redesign with DevTools styling #19707

taneliang · 2020-08-27T10:01:23Z

Summary

Adopt DevTools styling in the scheduling profiler and show a persistent toolbar at the top of the page.

Resolves #19676.

High-Level Goals

Design: Match React DevTools's design, so that we carry the same brand and level of professionalism.

Code: Revamp App code structure and CSS to resemble the DevTools component -- this is intended to make it slightly easier to merge the scheduling profiler into DevTools when the time comes.

Future expansion: Create space on the canvas page to display the React version (with a view towards #19668) and other profile metadata e.g. profile name and browser type.

Misc features:

Display an import button even after a profile has been loaded.
Add an import error dialog.

Implementation Breakdown

This is a large PR, but a lot of the code is copied verbatim from react-devtools-shared.

Replaces the existing ImportPage, mainly with ImportButton
Fork these from react-devtools-shared unchanged: Button, ButtonIcon, Icon, ModalDialog, ReactLogo, TabBar, Tooltip, root.css, utils/storage.js
Fork these from react-devtools-shared with modifications:
- Settings: I've modified this to remove its dependency on BridgeContext and StoreContext, both of which are not used in the scheduling profiler. I've left the list of CSS variables unchanged even though most of them aren't used as far as I can tell.
- hooks.js: Removed unused hooks and add a useBrowserTheme hook for auto dark mode 🎉
Inject a new DEVTOOLS_VERSION variable similar to DevTools', but computed from the scheduling profiler's package.json instead.
Overhaul App.js with the rough component hierarchy and same CSS styles as DevTools.js
Create a new SchedulingProfiler component following Profiler.js.

If we decide to accept this PR, we'll want to adapt #19690's help text to the new design and code structure.

Questions

Should we import code from react-devtools-shared instead of forking their implementations?
Are there any standard sources for the DevTools icons? I did a cursory search and found an icon in the Chromium source code, but I'm not sure where the others are from.
If we want to continue forking the implementations, should we delete unused code or leave them around? I'd imagine that leaving them around will ease the eventual merging as the diff will be minimized.

TODO

Resolve questions above
Place dark mode behind a new feature flag; the profiler canvas does not support dark mode. We'll leave comfortable sizing there; even though the canvas doesn't support comfortable sizing, I think people may appreciate having the rest of the UI text scale.

Test Plan

Before import (equivalent to the old ImportPage):

Before	After

After import:

Before	After

Because we're using DevTools code, there's built-in support for configurable (auto!) dark mode and display density. Unfortunately, this is not very useful now because the profiler canvas doesn't have support for CSS variables yet. I'll hide it behind a feature flag, but this is what it looks like now:

Settings page:

Comfortable dark:

Compact dark:

Comfortable dark:

Hidden image when viewport is shorter than 600px:

New import failure dialog:

Known Issues

It's possible to hover over the canvas content even if the settings dialog is up. I can fix that in a separate PR to prevent this PR from bloating further.

codesandbox-ci · 2020-08-27T10:04:46Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 40a5d22:

Sandbox	Source
React	Configuration

sizebot · 2020-08-27T10:07:11Z

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 40a5d22

sizebot · 2020-08-27T10:07:27Z

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 40a5d22

bvaughn · 2020-09-01T13:43:23Z

This is in my queue to review. I was on PTO last week though and I have a lot of catch up to do. I'll try to get to it sometime in the next couple of days. 😄

There is a Yarn lockfile conflict that might be nice to fix when you get the chance though.

taneliang · 2020-09-02T13:42:53Z

No problem, take your time! 😄

I've rebased on master and fixed the conflict, and made 2 tiny changes:

Bumped the Reach packages to their latest versions
Removed the version string from the toolbar as I doubt users will want/need to see it. The version number can still be found in the settings panel.

taneliang · 2020-09-02T14:16:16Z

Also hid the dark mode setting behind a new scheduling profiler feature flag enableDarkMode.

App no longer goes into dark mode even if the app was previously set to Auto or Dark

Theme setting is now also hidden

Dark mode still available when enableDarkMode = true

taneliang · 2020-09-03T13:24:36Z

Oops, conflicts! I'll rebase this on master

Bug: suppose we have a file which causes the profiler to display an error when imported. Importing the file once causes the error to be displayed correctly. However, if the file is imported a second time, the error is not displayed again. This happens because we listen to the `input` element's `onChange` event. Selecting the same file a second time does not cause the element's value to change, so the `handleFiles` callback is not called. This commit fixes this bug by emptying the `input` element's `value` in `handleFiles`. This bug is also present in the React DevTools Profiler.

bvaughn · 2020-09-03T13:44:15Z

Should we import code from react-devtools-shared instead of forking their implementations?

Importing seems nice!

Are there any standard sources for the DevTools icons? I did a cursory search and found an icon in the Chromium source code, but I'm not sure where the others are from.

Most of them come from Material UI: https://material-ui.com/components/material-icons/

If we want to continue forking the implementations, should we delete unused code or leave them around? I'd imagine that leaving them around will ease the eventual merging as the diff will be minimized.

No strong preference, but I also think using react-devtools-shared seems nice so maybe this question doesn't apply.

bvaughn

I'm going to sit down the review now since most of what I'm writing is "use the shared thing rather than fork" 😁 I'd be happy to make that change myself, or you could. Your call!

bvaughn · 2020-09-03T13:48:05Z

packages/react-devtools-scheduling-profiler/src/utils/storage.js

+  try {
+    return sessionStorage.setItem(key, value);
+  } catch (error) {}
+}


We should just use react-devtools-shared/src/storage.js.

bvaughn · 2020-09-03T13:49:37Z

packages/react-devtools-scheduling-profiler/src/root.css

+  /* Constant values shared between JS and CSS */
+  --interaction-commit-size: 10px;
+  --interaction-label-width: 200px;
+}


Does not feel good to fork these styles. Too easy for them to get out of sync.

Also feels a little weird to import react-devtools-shared/src/devtools/views/root.css but that seems better as long as we eventually plan to merge this profiler into the main DevTools app.

bvaughn · 2020-09-03T13:51:04Z

packages/react-devtools-scheduling-profiler/src/index.js

@@ -19,6 +19,7 @@ import './index.css';

 const container = document.createElement('div');
 container.id = 'root';
+container.style.height = '100%'; // Style specified here as CSS for #root will be stripped


nit: Could do

import styles from './index.css'; const container = document.createElement('div'); container.className = styles.Container; container.id = 'root';

bvaughn · 2020-09-03T13:51:30Z

packages/react-devtools-scheduling-profiler/src/hooks.js

+  }, []);
+
+  return theme;
+}


Let's use react-devtools-shared/src/devtools/views/hooks.js for all of these but useBrowserTheme?

bvaughn · 2020-09-03T13:52:43Z

packages/react-devtools-scheduling-profiler/src/constants.js

+  COMPACT_LINE_HEIGHT = 10;
+}
+
+export {COMFORTABLE_LINE_HEIGHT, COMPACT_LINE_HEIGHT};


Can we just import these from react-devtools-shared/src/constants.js?

or e.g.

export {COMFORTABLE_LINE_HEIGHT, COMPACT_LINE_HEIGHT} from 'react-devtools-shared/src/constants.js';

bvaughn · 2020-09-03T13:53:12Z

packages/react-devtools-scheduling-profiler/src/constants.js

+
+try {
+  // $FlowFixMe
+  const rawStyleString = require('!!raw-loader!react-devtools-shared/src/devtools/views/root.css')


Whoops, we aren't even using our own forked root CSS here 😆

bvaughn · 2020-09-03T14:05:24Z

packages/react-devtools-scheduling-profiler/src/hooks.js

+    };
+
+    const mediaQueryList = window.matchMedia('(prefers-color-scheme: dark)');
+    mediaQueryList.addEventListener('change', handlePreferredColorSchemeChange);


I don't think this picks up on dark mode if it's already set (only if the user changes their system theme after the profiler has been opened). At least that seems to be the behavior on OSX (the "change" event won't get fired).

We could also check window.matchMedia(DARK_MODE_QUERY).matches in the effect and update, but then we might have to render twice (once for the default "light" and then again for "dark").

I think this might be a nice opportunity to use the useMutableSource hook! 😄 It will handle all of this for us. Here's how you'd use it:

import { unstable_createMutableSource as createMutableSource, unstable_useMutableSource as useMutableSource, } from 'react'; export type BrowserTheme = 'dark' | 'light'; const DARK_MODE_QUERY = '(prefers-color-scheme: dark)'; const getSnapshot = window => window.matchMedia(DARK_MODE_QUERY).matches ? 'dark' : 'light'; const darkModeMutableSource = createMutableSource( window, () => window.matchMedia(DARK_MODE_QUERY).matches, ); const subscribe = (window, callback) => { const mediaQueryList = window.matchMedia(DARK_MODE_QUERY); mediaQueryList.addEventListener('change', callback); return () => { mediaQueryList.removeEventListener('change', callback); }; }; export function useBrowserTheme(): BrowserTheme { return useMutableSource(darkModeMutableSource, getSnapshot, subscribe); }

Wow, cool! I didn't know this existed 😆

It's not released yet in stable, but DevTools uses prerelease APIs 😄

https://github.com/reactjs/rfcs/blob/master/text/0147-use-mutable-source.md

bvaughn · 2020-09-03T14:21:50Z

packages/react-devtools-scheduling-profiler/src/TabBar.js

@@ -0,0 +1,133 @@
+/**


Let's use react-devtools-shared/src/devtools/views/TabBar.js and react-devtools-shared/src/devtools/views/TabBar.css rather than fork.

bvaughn · 2020-09-03T14:28:26Z

Eh, screw it 😄 I'll hop in and help since it's after hours for you. Give me a few minutes and I'll push a commit that implements my own nits.

bvaughn · 2020-09-03T14:47:30Z

packages/react-devtools-scheduling-profiler/src/App.js

-import ImportPage from './ImportPage';
-import CanvasPage from './CanvasPage';
+import {ModalDialogContextController} from './ModalDialog';
+import {SettingsContextController} from './Settings/SettingsContext';


I'm not convinced the settings dialog is worth the hassle of the forked contexts and files. For now, I'm going to just rip it out :D

taneliang · 2020-09-03T14:56:17Z

Give me a few minutes and I'll push a commit that implements my own nits.

Thanks! I gotta get back to my art history class readings 😆 Let me know if there's anything I can do! I'll be happy to follow up on anything tomorrow afternoon.

bvaughn

Hope it's okay that I DRYed it up a bit.

We could still do more (with the context package and useContextMenu hook) but that can come later.

taneliang · 2020-09-03T16:01:25Z

Yeah! This is much better.

I just pushed a commit to call updateThemeVariables when enableDarkMode is false, to fix this:

bvaughn · 2020-09-03T16:03:27Z

packages/react-devtools-scheduling-profiler/src/hooks.js

@@ -53,6 +52,8 @@ export function useBrowserTheme(): void {
        default:
          throw Error(`Unsupported theme value "${theme}"`);
      }
+    } else {
+      updateThemeVariables('light', documentElements);


@taneliang This should be theme not 'light' right?

Ah this was outside of the feature flag conditional. My apologies.

Oh I think we want 'light' to prevent the app from going into dark mode?

Edit: yep! Sorry, ignore this comment, your second comment didn't load until I commented

Co-authored-by: Brian Vaughn <[email protected]>

facebook-github-bot added the CLA Signed label Aug 27, 2020

taneliang changed the title ~~Scheduling Profiler: RFC: Redesign with DevTools styling~~ Scheduling Profiler: Redesign with DevTools styling Aug 27, 2020

kartikcho mentioned this pull request Aug 27, 2020

Scheduling Profiler: Add How-To, loader & some UI tweaks #19690

Closed

bvaughn self-assigned this Aug 31, 2020

taneliang force-pushed the devtools-styles branch from 6625d09 to 5aa5eda Compare September 2, 2020 13:37

kartikcho mentioned this pull request Sep 3, 2020

Scheduling Profiler: Add Fast Refresh #19757

Merged

taneliang mentioned this pull request Sep 3, 2020

Scheduling Profiler: Move preprocessing to web worker and add loading indicator #19759

Merged

taneliang added 2 commits September 3, 2020 21:23

Replace ImportPage with DevTools UI and toolbar

0dcbf71

Fix tooltips disappearing on data replacement

d545781

taneliang added 6 commits September 3, 2020 21:28

Add @reach packages to scheduling profiler package.json

9621a5d

Hide screenshot when viewport height <600px

482fbc7

Upgrade @reach packages to latest patch version

129d0d0

Remove app version from toolbar

f9bc6a2

Hide dark mode behind feature flag

583522b

taneliang force-pushed the devtools-styles branch from 98a9c72 to 2592245 Compare September 3, 2020 13:30

bvaughn reviewed Sep 3, 2020

View reviewed changes

Removed a lot of forked files and made a few minor tweaks

dce4389

bvaughn approved these changes Sep 3, 2020

View reviewed changes

Fix theme variable load when enableDarkMode=false

40a5d22

bvaughn reviewed Sep 3, 2020

View reviewed changes

bvaughn merged commit 38a512a into facebook:master Sep 3, 2020

taneliang deleted the devtools-styles branch September 4, 2020 14:58

koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021

Scheduling Profiler: Redesign with DevTools styling (facebook#19707)

1c0167a

Co-authored-by: Brian Vaughn <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduling Profiler: Redesign with DevTools styling #19707

Scheduling Profiler: Redesign with DevTools styling #19707

taneliang commented Aug 27, 2020 •

edited by bvaughn

Loading

codesandbox-ci bot commented Aug 27, 2020 •

edited

Loading

sizebot commented Aug 27, 2020 •

edited

Loading

sizebot commented Aug 27, 2020 •

edited

Loading

bvaughn commented Sep 1, 2020

taneliang commented Sep 2, 2020

taneliang commented Sep 2, 2020

taneliang commented Sep 3, 2020 •

edited

Loading

bvaughn commented Sep 3, 2020

bvaughn left a comment

bvaughn Sep 3, 2020 •

edited

Loading

bvaughn Sep 3, 2020

bvaughn Sep 3, 2020

bvaughn Sep 3, 2020

bvaughn Sep 3, 2020

bvaughn Sep 3, 2020

bvaughn Sep 3, 2020 •

edited

Loading

taneliang Sep 3, 2020

bvaughn Sep 3, 2020

bvaughn Sep 3, 2020

bvaughn commented Sep 3, 2020 •

edited

Loading

bvaughn Sep 3, 2020

taneliang commented Sep 3, 2020

bvaughn left a comment •

edited

Loading

taneliang commented Sep 3, 2020 •

edited

Loading

bvaughn Sep 3, 2020

bvaughn Sep 3, 2020

taneliang Sep 3, 2020 •

edited

Loading

Scheduling Profiler: Redesign with DevTools styling #19707

Scheduling Profiler: Redesign with DevTools styling #19707

Conversation

taneliang commented Aug 27, 2020 • edited by bvaughn Loading

Summary

High-Level Goals

Implementation Breakdown

Questions

TODO

Test Plan

Known Issues

codesandbox-ci bot commented Aug 27, 2020 • edited Loading

sizebot commented Aug 27, 2020 • edited Loading

Size changes (stable)

sizebot commented Aug 27, 2020 • edited Loading

Size changes (experimental)

bvaughn commented Sep 1, 2020

taneliang commented Sep 2, 2020

taneliang commented Sep 2, 2020

taneliang commented Sep 3, 2020 • edited Loading

bvaughn commented Sep 3, 2020

bvaughn left a comment

Choose a reason for hiding this comment

bvaughn Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

taneliang commented Sep 3, 2020

bvaughn left a comment • edited Loading

Choose a reason for hiding this comment

taneliang commented Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taneliang Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

taneliang commented Aug 27, 2020 •

edited by bvaughn

Loading

codesandbox-ci bot commented Aug 27, 2020 •

edited

Loading

sizebot commented Aug 27, 2020 •

edited

Loading

sizebot commented Aug 27, 2020 •

edited

Loading

taneliang commented Sep 3, 2020 •

edited

Loading

bvaughn Sep 3, 2020 •

edited

Loading

bvaughn Sep 3, 2020 •

edited

Loading

bvaughn commented Sep 3, 2020 •

edited

Loading

bvaughn left a comment •

edited

Loading

taneliang commented Sep 3, 2020 •

edited

Loading

taneliang Sep 3, 2020 •

edited

Loading