From 9e5575cf6909b85c0321824e0b13276c3ddf8f6d Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Wed, 6 Oct 2021 09:08:47 -0700 Subject: [PATCH] Dialog management cleanup Summary: This diff moves the dialogs * Settings * Plugin Manager * Doctor * Sign in * Changelog To use the imperative dialog APIs, rather then organising them through reducers which adds a lot of indirection which isn't really needed but hard to follow. Reviewed By: passy Differential Revision: D30192002 fbshipit-source-id: ba38b2e700da3e442653786448fcbf85074981ad --- desktop/app/src/MenuBar.tsx | 24 +++++++++----- .../app/src/chrome/ExportDataPluginSheet.tsx | 1 - desktop/app/src/chrome/SheetRenderer.tsx | 23 ------------- .../app/src/chrome/fb-stubs/SignInSheet.tsx | 6 ++-- desktop/app/src/deeplink.tsx | 11 ++----- .../dispatcher/handleOpenPluginDeeplink.tsx | 4 +-- desktop/app/src/reducers/application.tsx | 24 ++------------ desktop/app/src/sandy-chrome/LeftRail.tsx | 15 +++------ desktop/app/src/sandy-chrome/SandyApp.tsx | 30 ++++++++++------- desktop/app/src/server/FlipperServerImpl.tsx | 15 +++------ .../flipper-plugin/src/__tests__/api.node.tsx | 1 + desktop/flipper-plugin/src/index.ts | 4 +-- desktop/flipper-plugin/src/ui/Dialog.tsx | 33 ++++++++++++++++++- .../src/utils/renderReactRoot.tsx | 26 +++++++++++++-- 14 files changed, 112 insertions(+), 105 deletions(-) diff --git a/desktop/app/src/MenuBar.tsx b/desktop/app/src/MenuBar.tsx index c35bc363f..3f4c67f02 100644 --- a/desktop/app/src/MenuBar.tsx +++ b/desktop/app/src/MenuBar.tsx @@ -16,12 +16,6 @@ import { startFileExport, startLinkExport, } from './utils/exportData'; -import { - setActiveSheet, - ACTIVE_SHEET_PLUGINS, - ACTIVE_SHEET_SETTINGS, - ACTIVE_SHEET_CHANGELOG, -} from './reducers/application'; import {setStaticView} from './reducers/connections'; import {Store} from './reducers/'; import electron, {MenuItemConstructorOptions} from 'electron'; @@ -33,11 +27,16 @@ import { _buildInMenuEntries, _wrapInteractionHandler, getFlipperLib, + Dialog, } from 'flipper-plugin'; import {StyleGuide} from './sandy-chrome/StyleGuide'; import {showEmulatorLauncher} from './sandy-chrome/appinspect/LaunchEmulator'; import {webFrame} from 'electron'; import {openDeeplinkDialog} from './deeplink'; +import React from 'react'; +import ChangelogSheet from './chrome/ChangelogSheet'; +import PluginManager from './chrome/plugin-manager/PluginManager'; +import SettingsSheet from './chrome/SettingsSheet'; export type DefaultKeyboardAction = keyof typeof _buildInMenuEntries; export type TopLevelMenu = 'Edit' | 'View' | 'Window' | 'Help'; @@ -253,7 +252,11 @@ function getTemplate( { label: 'Preferences', accelerator: 'Cmd+,', - click: () => store.dispatch(setActiveSheet(ACTIVE_SHEET_SETTINGS)), + click: () => { + Dialog.showModal((onHide) => ( + + )); + }, }, { label: 'Import Flipper File...', @@ -343,9 +346,12 @@ function getTemplate( { label: 'Manage Plugins...', click: function () { - store.dispatch(setActiveSheet(ACTIVE_SHEET_PLUGINS)); + Dialog.showModal((onHide) => ); }, }, + { + type: 'separator', + }, { label: 'Flipper style guide', click() { @@ -404,7 +410,7 @@ function getTemplate( { label: 'Changelog', click() { - store.dispatch(setActiveSheet(ACTIVE_SHEET_CHANGELOG)); + Dialog.showModal((onHide) => ); }, }, ]; diff --git a/desktop/app/src/chrome/ExportDataPluginSheet.tsx b/desktop/app/src/chrome/ExportDataPluginSheet.tsx index 3a308d905..94e9ba550 100644 --- a/desktop/app/src/chrome/ExportDataPluginSheet.tsx +++ b/desktop/app/src/chrome/ExportDataPluginSheet.tsx @@ -47,7 +47,6 @@ type DispatchFromProps = { type Props = OwnProps & StateFromProps & DispatchFromProps; const Container = styled(FlexColumn)({ - width: 700, maxHeight: 700, padding: 8, }); diff --git a/desktop/app/src/chrome/SheetRenderer.tsx b/desktop/app/src/chrome/SheetRenderer.tsx index 465248e07..5108735e7 100644 --- a/desktop/app/src/chrome/SheetRenderer.tsx +++ b/desktop/app/src/chrome/SheetRenderer.tsx @@ -9,26 +9,15 @@ import React, {useCallback} from 'react'; import ShareSheetExportUrl from './ShareSheetExportUrl'; -import SignInSheet from './fb-stubs/SignInSheet'; import ExportDataPluginSheet from './ExportDataPluginSheet'; import ShareSheetExportFile from './ShareSheetExportFile'; import Sheet from './Sheet'; import { - ACTIVE_SHEET_PLUGINS, ACTIVE_SHEET_SHARE_DATA, - ACTIVE_SHEET_SIGN_IN, - ACTIVE_SHEET_SETTINGS, - ACTIVE_SHEET_DOCTOR, ACTIVE_SHEET_SHARE_DATA_IN_FILE, ACTIVE_SHEET_SELECT_PLUGINS_TO_EXPORT, - ACTIVE_SHEET_CHANGELOG, - ACTIVE_SHEET_CHANGELOG_RECENT_ONLY, } from '../reducers/application'; import {Logger} from '../fb-interfaces/Logger'; -import PluginManager from './plugin-manager/PluginManager'; -import SettingsSheet from './SettingsSheet'; -import DoctorSheet from './DoctorSheet'; -import ChangelogSheet from './ChangelogSheet'; import {useStore} from '../utils/useStore'; export function SheetRenderer({logger}: {logger: Logger}) { @@ -39,18 +28,6 @@ export function SheetRenderer({logger}: {logger: Logger}) { const renderSheet = useCallback( (onHide: () => any) => { switch (activeSheet) { - case ACTIVE_SHEET_PLUGINS: - return ; - case ACTIVE_SHEET_SIGN_IN: - return ; - case ACTIVE_SHEET_SETTINGS: - return ; - case ACTIVE_SHEET_DOCTOR: - return ; - case ACTIVE_SHEET_CHANGELOG: - return ; - case ACTIVE_SHEET_CHANGELOG_RECENT_ONLY: - return ; case ACTIVE_SHEET_SELECT_PLUGINS_TO_EXPORT: return ; case ACTIVE_SHEET_SHARE_DATA: diff --git a/desktop/app/src/chrome/fb-stubs/SignInSheet.tsx b/desktop/app/src/chrome/fb-stubs/SignInSheet.tsx index 291a57d21..5cc7cc704 100644 --- a/desktop/app/src/chrome/fb-stubs/SignInSheet.tsx +++ b/desktop/app/src/chrome/fb-stubs/SignInSheet.tsx @@ -7,6 +7,8 @@ * @format */ -export default function SignInSheetFunc(_: {onHide: () => any}) { - return null; +export async function showLoginDialog( + _initialToken: string = '', +): Promise { + return false; } diff --git a/desktop/app/src/deeplink.tsx b/desktop/app/src/deeplink.tsx index 09de7b29f..8ee8471d4 100644 --- a/desktop/app/src/deeplink.tsx +++ b/desktop/app/src/deeplink.tsx @@ -7,11 +7,6 @@ * @format */ -import { - ACTIVE_SHEET_SIGN_IN, - setActiveSheet, - setPastedToken, -} from './reducers/application'; import {Group, SUPPORTED_GROUPS} from './reducers/supportForm'; import {Logger} from './fb-interfaces/Logger'; import {Store} from './reducers/index'; @@ -20,6 +15,7 @@ import {selectPlugin, getAllClients} from './reducers/connections'; import {Dialog} from 'flipper-plugin'; import {handleOpenPluginDeeplink} from './dispatcher/handleOpenPluginDeeplink'; import {message} from 'antd'; +import {showLoginDialog} from './chrome/fb-stubs/SignInSheet'; import {track} from './deeplinkTracking'; const UNKNOWN = 'Unknown deeplink'; @@ -81,10 +77,7 @@ export async function handleDeeplink( throw unknownError(); } else if (uri.pathname.match(/^\/*login\/*$/)) { const token = uri.searchParams.get('token'); - store.dispatch(setPastedToken(token ?? undefined)); - if (store.getState().application.activeSheet !== ACTIVE_SHEET_SIGN_IN) { - store.dispatch(setActiveSheet(ACTIVE_SHEET_SIGN_IN)); - } + showLoginDialog(token ?? ''); return; } const match = uriComponents(query); diff --git a/desktop/app/src/dispatcher/handleOpenPluginDeeplink.tsx b/desktop/app/src/dispatcher/handleOpenPluginDeeplink.tsx index 94b69d3f1..f7d18add1 100644 --- a/desktop/app/src/dispatcher/handleOpenPluginDeeplink.tsx +++ b/desktop/app/src/dispatcher/handleOpenPluginDeeplink.tsx @@ -13,7 +13,6 @@ import {getUser} from '../fb-stubs/user'; import {State, Store} from '../reducers/index'; import {checkForUpdate} from '../fb-stubs/checkForUpdate'; import {getAppVersion} from '../utils/info'; -import {ACTIVE_SHEET_SIGN_IN, setActiveSheet} from '../reducers/application'; import {UserNotSignedInError} from '../utils/errors'; import {selectPlugin, setPluginEnabled} from '../reducers/connections'; import {getUpdateAvailableMessage} from '../chrome/UpdateIndicator'; @@ -29,6 +28,7 @@ import Client from '../Client'; import {RocketOutlined} from '@ant-design/icons'; import {showEmulatorLauncher} from '../sandy-chrome/appinspect/LaunchEmulator'; import {getAllClients} from '../reducers/connections'; +import {showLoginDialog} from '../chrome/fb-stubs/SignInSheet'; import {DeeplinkInteraction, OpenPluginParams} from '../deeplinkTracking'; export function parseOpenPluginParams(query: string): OpenPluginParams { @@ -233,7 +233,7 @@ async function showPleaseLoginDialog( return false; } - store.dispatch(setActiveSheet(ACTIVE_SHEET_SIGN_IN)); + await showLoginDialog(); // wait until login succeeded await waitForLogin(store); return true; diff --git a/desktop/app/src/reducers/application.tsx b/desktop/app/src/reducers/application.tsx index 33dc85dcc..b19070277 100644 --- a/desktop/app/src/reducers/application.tsx +++ b/desktop/app/src/reducers/application.tsx @@ -14,7 +14,6 @@ import {v1 as uuidv1} from 'uuid'; import {ReactElement} from 'react'; import CancellableExportStatus from '../chrome/CancellableExportStatus'; import {Actions} from './'; -import produce from 'immer'; export const ACTIVE_SHEET_PLUGINS: 'PLUGINS' = 'PLUGINS'; export const ACTIVE_SHEET_SELECT_PLUGINS_TO_EXPORT: 'SELECT_PLUGINS_TO_EXPORT' = 'SELECT_PLUGINS_TO_EXPORT'; @@ -31,16 +30,13 @@ export const ACTIVE_SHEET_CHANGELOG = 'ACTIVE_SHEET_CHANGELOG'; export const ACTIVE_SHEET_CHANGELOG_RECENT_ONLY = 'ACTIVE_SHEET_CHANGELOG_RECENT_ONLY'; +/** + * @deprecated this is a weird mechanism, and using imperative dialogs will be simpler + */ export type ActiveSheet = - | typeof ACTIVE_SHEET_PLUGINS | typeof ACTIVE_SHEET_SHARE_DATA - | typeof ACTIVE_SHEET_SIGN_IN - | typeof ACTIVE_SHEET_SETTINGS - | typeof ACTIVE_SHEET_DOCTOR | typeof ACTIVE_SHEET_SHARE_DATA_IN_FILE | typeof ACTIVE_SHEET_SELECT_PLUGINS_TO_EXPORT - | typeof ACTIVE_SHEET_CHANGELOG - | typeof ACTIVE_SHEET_CHANGELOG_RECENT_ONLY | null; export type LauncherMsg = { @@ -84,7 +80,6 @@ export type State = { altServerPorts: ServerPorts; launcherMsg: LauncherMsg; statusMessages: Array; - pastedToken?: string; }; type BooleanActionType = @@ -152,10 +147,6 @@ export type Action = | { type: 'REMOVE_STATUS_MSG'; payload: {msg: string; sender: string}; - } - | { - type: 'SET_PASTED_TOKEN'; - payload?: string; }; export const initialState: () => State = () => ({ @@ -294,10 +285,6 @@ export default function reducer( return {...state, statusMessages}; } return state; - } else if (action.type === 'SET_PASTED_TOKEN') { - return produce(state, (draft) => { - draft.pastedToken = action.payload; - }); } else { return state; } @@ -371,8 +358,3 @@ export const removeStatusMessage = (payload: StatusMessageType): Action => ({ type: 'REMOVE_STATUS_MSG', payload, }); - -export const setPastedToken = (pastedToken?: string): Action => ({ - type: 'SET_PASTED_TOKEN', - payload: pastedToken, -}); diff --git a/desktop/app/src/sandy-chrome/LeftRail.tsx b/desktop/app/src/sandy-chrome/LeftRail.tsx index 3f90c6467..a268e2416 100644 --- a/desktop/app/src/sandy-chrome/LeftRail.tsx +++ b/desktop/app/src/sandy-chrome/LeftRail.tsx @@ -24,13 +24,10 @@ import { import {SidebarLeft, SidebarRight} from './SandyIcons'; import {useDispatch, useStore} from '../utils/useStore'; import { - ACTIVE_SHEET_PLUGINS, - ACTIVE_SHEET_SIGN_IN, - setActiveSheet, toggleLeftSidebarVisible, toggleRightSidebarVisible, } from '../reducers/application'; -import {theme, Layout, withTrackingScope} from 'flipper-plugin'; +import {theme, Layout, withTrackingScope, Dialog} from 'flipper-plugin'; import SetupDoctorScreen, {checkHasNewProblem} from './SetupDoctorScreen'; import SettingsSheet from '../chrome/SettingsSheet'; import WelcomeScreen from './WelcomeScreen'; @@ -51,6 +48,8 @@ import isProduction from '../utils/isProduction'; import NetworkGraph from '../chrome/NetworkGraph'; import FpsGraph from '../chrome/FpsGraph'; import UpdateIndicator from '../chrome/UpdateIndicator'; +import PluginManager from '../chrome/plugin-manager/PluginManager'; +import {showLoginDialog} from '../chrome/fb-stubs/SignInSheet'; const LeftRailButtonElem = styled(Button)<{kind?: 'small'}>(({kind}) => ({ width: kind === 'small' ? 32 : 36, @@ -119,7 +118,6 @@ export const LeftRail = withTrackingScope(function LeftRail({ toplevelSelection, setToplevelSelection, }: ToplevelProps) { - const dispatch = useDispatch(); return ( @@ -136,7 +134,7 @@ export const LeftRail = withTrackingScope(function LeftRail({ icon={} title="Plugin Manager" onClick={() => { - dispatch(setActiveSheet(ACTIVE_SHEET_PLUGINS)); + Dialog.showModal((onHide) => ); }} /> state.user); const login = (user?.id ?? null) !== null; const profileUrl = user?.profile_picture?.uri; - const showLogin = useCallback(() => { - dispatch(setActiveSheet(ACTIVE_SHEET_SIGN_IN)); - }, [dispatch]); const [showLogout, setShowLogout] = useState(false); const onHandleVisibleChange = useCallback( (visible) => setShowLogout(visible), @@ -398,7 +393,7 @@ function LoginButton() { } title="Log In" - onClick={showLogin} + onClick={() => showLoginDialog()} /> ); diff --git a/desktop/app/src/sandy-chrome/SandyApp.tsx b/desktop/app/src/sandy-chrome/SandyApp.tsx index 042c81bcc..fb04f703e 100644 --- a/desktop/app/src/sandy-chrome/SandyApp.tsx +++ b/desktop/app/src/sandy-chrome/SandyApp.tsx @@ -8,7 +8,14 @@ */ import React, {useEffect, useState, useCallback} from 'react'; -import {TrackingScope, useLogger, _Sidebar, Layout} from 'flipper-plugin'; +import { + TrackingScope, + useLogger, + _Sidebar, + Layout, + Dialog, + _PortalsManager, +} from 'flipper-plugin'; import {Link, styled} from '../ui'; import {theme} from 'flipper-plugin'; import {ipcRenderer} from 'electron'; @@ -18,17 +25,13 @@ import {LeftRail} from './LeftRail'; import {useStore, useDispatch} from '../utils/useStore'; import {FlipperDevTools} from '../chrome/FlipperDevTools'; import {setStaticView} from '../reducers/connections'; -import { - ACTIVE_SHEET_CHANGELOG_RECENT_ONLY, - setActiveSheet, - toggleLeftSidebarVisible, -} from '../reducers/application'; +import {toggleLeftSidebarVisible} from '../reducers/application'; import {AppInspect} from './appinspect/AppInspect'; import PluginContainer from '../PluginContainer'; import {ContentContainer} from './ContentContainer'; import {Notification} from './notification/Notification'; import {SheetRenderer} from '../chrome/SheetRenderer'; -import {hasNewChangesToShow} from '../chrome/ChangelogSheet'; +import ChangelogSheet, {hasNewChangesToShow} from '../chrome/ChangelogSheet'; import {getVersionString} from '../utils/versionString'; import config from '../fb-stubs/config'; import {WelcomeScreenStaticView} from './WelcomeScreen'; @@ -94,7 +97,7 @@ export function SandyApp() { registerStartupTime(logger); if (hasNewChangesToShow(window.localStorage)) { - dispatch(setActiveSheet(ACTIVE_SHEET_CHANGELOG_RECENT_ONLY)); + Dialog.showModal((onHide) => ); } // don't warn about logger, even with a new logger we don't want to re-register // eslint-disable-next-line @@ -122,7 +125,9 @@ export function SandyApp() { }); } }) - .catch((e) => console.error('isEmployee check failed:', e)); + .catch((e) => { + console.warn('Failed to check if user is employee', e); + }); } }, []); @@ -134,8 +139,7 @@ export function SandyApp() { ) : null; return ( - - + - + + <_PortalsManager /> + ); } diff --git a/desktop/app/src/server/FlipperServerImpl.tsx b/desktop/app/src/server/FlipperServerImpl.tsx index c340ff6f3..8c256ac6c 100644 --- a/desktop/app/src/server/FlipperServerImpl.tsx +++ b/desktop/app/src/server/FlipperServerImpl.tsx @@ -17,11 +17,7 @@ import {CertificateExchangeMedium} from './utils/CertificateProvider'; import {isLoggedIn} from '../fb-stubs/user'; import React from 'react'; import {Typography} from 'antd'; -import { - ACTIVE_SHEET_SIGN_IN, - ServerPorts, - setActiveSheet, -} from '../reducers/application'; +import {ServerPorts} from '../reducers/application'; import {AndroidDeviceManager} from './devices/android/androidDeviceManager'; import {IOSDeviceManager} from './devices/ios/iOSDeviceManager'; import metroDevice from './devices/metro/metroDeviceManager'; @@ -35,6 +31,7 @@ import { import {ServerDevice} from './devices/ServerDevice'; import {Base64} from 'js-base64'; import MetroDevice from './devices/metro/MetroDevice'; +import {showLoginDialog} from '../chrome/fb-stubs/SignInSheet'; export interface FlipperServerConfig { enableAndroid: boolean; @@ -135,11 +132,9 @@ export class FlipperServerImpl implements FlipperServer { <> and{' '} - this.store.dispatch( - setActiveSheet(ACTIVE_SHEET_SIGN_IN), - ) - }> + onClick={() => { + showLoginDialog(); + }}> log in to Facebook Intern diff --git a/desktop/flipper-plugin/src/__tests__/api.node.tsx b/desktop/flipper-plugin/src/__tests__/api.node.tsx index 5314cd269..08e10d116 100644 --- a/desktop/flipper-plugin/src/__tests__/api.node.tsx +++ b/desktop/flipper-plugin/src/__tests__/api.node.tsx @@ -85,6 +85,7 @@ test('Correct top level API exposed', () => { "DeviceLogEntry", "DeviceLogListener", "DevicePluginClient", + "DialogResult", "Draft", "ElementAttribute", "ElementData", diff --git a/desktop/flipper-plugin/src/index.ts b/desktop/flipper-plugin/src/index.ts index b970e0d01..2953d64d2 100644 --- a/desktop/flipper-plugin/src/index.ts +++ b/desktop/flipper-plugin/src/index.ts @@ -61,7 +61,7 @@ export {Toolbar} from './ui/Toolbar'; export {MasterDetail} from './ui/MasterDetail'; export {CodeBlock} from './ui/CodeBlock'; -export {renderReactRoot} from './utils/renderReactRoot'; +export {renderReactRoot, _PortalsManager} from './utils/renderReactRoot'; export { Tracked, TrackingScope, @@ -115,7 +115,7 @@ export { } from './ui/data-inspector/DataDescription'; export {MarkerTimeline} from './ui/MarkerTimeline'; export {DataInspector} from './ui/data-inspector/DataInspector'; -export {Dialog} from './ui/Dialog'; +export {Dialog, DialogResult} from './ui/Dialog'; export { ElementsInspector, Element as ElementsInspectorElement, diff --git a/desktop/flipper-plugin/src/ui/Dialog.tsx b/desktop/flipper-plugin/src/ui/Dialog.tsx index 05c81b7e2..5120074c1 100644 --- a/desktop/flipper-plugin/src/ui/Dialog.tsx +++ b/desktop/flipper-plugin/src/ui/Dialog.tsx @@ -13,7 +13,8 @@ import React from 'react'; import {renderReactRoot} from '../utils/renderReactRoot'; import {Layout} from './Layout'; import {Spinner} from './Spinner'; -type DialogResult = Promise & {close: () => void}; + +export type DialogResult = Promise & {close: () => void}; type BaseDialogOptions = { title: string; @@ -104,6 +105,36 @@ export const Dialog = { ); }, + /** + * Shows an item in the modal stack, but without providing any further UI, like .show does. + */ + showModal( + fn: (hide: (result?: T) => void) => React.ReactElement, + ): DialogResult { + let cancel: () => void; + + return Object.assign( + new Promise((resolve) => { + renderReactRoot((hide) => { + cancel = () => { + hide(); + resolve(false); + }; + + return fn((result?: T) => { + hide(); + resolve(result ?? false); + }); + }); + }), + { + close() { + cancel(); + }, + }, + ); + }, + confirm({ message, onConfirm, diff --git a/desktop/flipper-plugin/src/utils/renderReactRoot.tsx b/desktop/flipper-plugin/src/utils/renderReactRoot.tsx index 3fa155e2e..12eab5de1 100644 --- a/desktop/flipper-plugin/src/utils/renderReactRoot.tsx +++ b/desktop/flipper-plugin/src/utils/renderReactRoot.tsx @@ -7,8 +7,9 @@ * @format */ -import React from 'react'; -import {render, unmountComponentAtNode} from 'react-dom'; +import {createState, useValue} from '../state/atom'; +import React, {ReactPortal} from 'react'; +import {createPortal, unmountComponentAtNode} from 'react-dom'; /** * This utility creates a fresh react render hook, which is great to render elements imperatively, like opening dialogs. @@ -20,9 +21,28 @@ export function renderReactRoot( // TODO: find a way to make this visible in unit tests as well const div = document.body.appendChild(document.createElement('div')); const unmount = () => { + portals.update((draft) => { + draft.delete(id); + }); unmountComponentAtNode(div); div.remove(); }; - render(handler(unmount), div); + const id = ++portalId; + const portal = createPortal(handler(unmount), div); + portals.update((draft) => { + draft.set(id, portal); + }); + return unmount; } + +let portalId = 0; +const portals = createState(new Map()); + +/** + * This is a dummy component, that just makes sure react roots are managed within a certain node in the main React tree, so that context etc is available. + */ +export function _PortalsManager() { + const portalElements = useValue(portals); + return <>{Array.from(portalElements).map(([_id, portal]) => portal)}; +}