From 0fe879c838c4d2cfda3001157c05023e6854521b Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Thu, 15 Apr 2021 07:46:28 -0700 Subject: [PATCH] Improve multiple element selector UI Summary: Layer selection is pretty easy to miss, as reported in for example: https://fb.workplace.com/groups/flippersupport/permalink/1098169193997071/ Moved the layer selection to the top of the view and gave it some highlighting + dynamic height. The section is no longer collapsible. Changelog: [Layout] Make the layer selection more prominent Reviewed By: priteshrnandgaonkar Differential Revision: D27708650 fbshipit-source-id: c86a55c3a20794aee86e64b6766b2ca4dd6b563f --- .../elements-inspector/ElementsInspector.tsx | 14 +++- .../src/ui/elements-inspector/elements.tsx | 37 +++------ desktop/plugins/public/layout/Inspector.tsx | 30 +++---- .../layout/MultipleSelectionSection.tsx | 80 ++++++------------- .../MultipleSelectionSection.node.tsx | 24 ------ 5 files changed, 62 insertions(+), 123 deletions(-) diff --git a/desktop/flipper-plugin/src/ui/elements-inspector/ElementsInspector.tsx b/desktop/flipper-plugin/src/ui/elements-inspector/ElementsInspector.tsx index 78c6e0b9e..466c81edd 100644 --- a/desktop/flipper-plugin/src/ui/elements-inspector/ElementsInspector.tsx +++ b/desktop/flipper-plugin/src/ui/elements-inspector/ElementsInspector.tsx @@ -10,6 +10,7 @@ import {Component} from 'react'; import {Elements, DecorateRow, ContextMenuExtension} from './elements'; import React from 'react'; +import {Layout} from '../Layout'; export type ElementID = string; @@ -72,11 +73,16 @@ export type ElementsInspectorProps = { alternateRowColor?: boolean; contextMenuExtensions?: () => Array; decorateRow?: DecorateRow; + /** + * By default the ElementsInspector takes all available space and is scrollab.e + */ + scrollable?: boolean; }; export class ElementsInspector extends Component { static defaultProps = { alternateRowColor: true, + scrollable: true, }; render() { const { @@ -91,9 +97,10 @@ export class ElementsInspector extends Component { alternateRowColor, contextMenuExtensions, decorateRow, + scrollable, } = this.props; - return ( + const renderedElems = ( { decorateRow={decorateRow} /> ); + if (scrollable) { + return {renderedElems}; + } else { + return renderedElems; + } } } diff --git a/desktop/flipper-plugin/src/ui/elements-inspector/elements.tsx b/desktop/flipper-plugin/src/ui/elements-inspector/elements.tsx index 3b1373dfe..b81f53036 100644 --- a/desktop/flipper-plugin/src/ui/elements-inspector/elements.tsx +++ b/desktop/flipper-plugin/src/ui/elements-inspector/elements.tsx @@ -449,8 +449,6 @@ function containsKeyInSearchResults( const ElementsContainer = styled('div')({ display: 'table', backgroundColor: theme.backgroundDefault, - minHeight: '100%', - minWidth: '100%', }); ElementsContainer.displayName = 'Elements:ElementsContainer'; @@ -489,7 +487,6 @@ export class Elements extends PureComponent { static defaultProps = { alternateRowColor: true, }; - _outerRef = React.createRef(); constructor(props: ElementsProps, context: Object) { super(props, context); this.state = { @@ -690,24 +687,14 @@ export class Elements extends PureComponent { }; scrollToSelectionRefHandler = (selectedRow: HTMLDivElement | null) => { - if ( - !selectedRow || - !this._outerRef.current || - this.props.selected === this.state.scrolledElement - ) { - return; - } - this.setState({scrolledElement: this.props.selected}); - const outer = this._outerRef.current; - if (outer.scrollTo) { - outer.scrollTo({ - top: this._calculateScrollTop( - outer.offsetHeight, - outer.scrollTop, - selectedRow.offsetHeight, - selectedRow.offsetTop, - ), + if (selectedRow && this.state.scrolledElement !== this.props.selected) { + // second child is the element containing the element name + // by scrolling to the second element, we make sure padding is addressed and we scroll horizontally as well + selectedRow?.children[1]?.scrollIntoView?.({ + block: 'center', + inline: 'center', }); + this.setState({scrolledElement: this.props.selected}); } }; @@ -761,7 +748,7 @@ export class Elements extends PureComponent { contextMenuExtensions={contextMenuExtensions} decorateRow={decorateRow} forwardedRef={ - selected == row.key && this.state.scrolledElement !== selected + selected == row.key // && this.state.scrolledElement !== selected ? this.scrollToSelectionRefHandler : null } @@ -771,11 +758,9 @@ export class Elements extends PureComponent { render() { return ( - - - {this.state.flatElements.map(this.buildRow)} - - + + {this.state.flatElements.map(this.buildRow)} + ); } } diff --git a/desktop/plugins/public/layout/Inspector.tsx b/desktop/plugins/public/layout/Inspector.tsx index b47219308..fc64144fd 100644 --- a/desktop/plugins/public/layout/Inspector.tsx +++ b/desktop/plugins/public/layout/Inspector.tsx @@ -13,19 +13,13 @@ import { PluginClient, ElementsInspector, ElementSearchResultSet, - FlexColumn, - styled, } from 'flipper'; import {debounce} from 'lodash'; import {Component} from 'react'; import {PersistedState, ElementMap} from './'; import React from 'react'; import MultipleSelectorSection from './MultipleSelectionSection'; - -const ElementsInspectorContainer = styled(FlexColumn)({ - width: '100%', - justifyContent: 'space-between', -}); +import {Layout} from 'flipper-plugin'; type GetNodesOptions = { force?: boolean; @@ -448,7 +442,17 @@ export default class Inspector extends Component { : this.state.elementSelector; return this.root() ? ( - + + {selectorData && selectorData.leaves.length > 1 ? ( + + ) : ( +
+ )} { focused={this.focused()} contextMenuExtensions={this.getAXContextMenuExtensions} /> - {selectorData && selectorData.leaves.length > 1 ? ( - - ) : null} - + ) : null; } } diff --git a/desktop/plugins/public/layout/MultipleSelectionSection.tsx b/desktop/plugins/public/layout/MultipleSelectionSection.tsx index d767f7484..2f7f01784 100644 --- a/desktop/plugins/public/layout/MultipleSelectionSection.tsx +++ b/desktop/plugins/public/layout/MultipleSelectionSection.tsx @@ -7,41 +7,16 @@ * @format */ -import { - FlexColumn, - FlexBox, - Element, - ElementID, - ElementsInspector, - Glyph, - colors, - styled, -} from 'flipper'; +import {Typography} from 'antd'; +import {Element, ElementID, ElementsInspector, styled} from 'flipper'; +import {Layout, theme} from 'flipper-plugin'; import React, {memo, useState} from 'react'; -const MultipleSelectorSectionContainer = styled(FlexColumn)({ - maxHeight: 100, -}); - -const MultipleSelectorSectionTitle = styled(FlexBox)({ - cursor: 'pointer', - backgroundColor: '#f6f7f9', - padding: '2px', - paddingLeft: '9px', - width: '325px', - height: '20px', - fontWeight: 500, - boxShadow: '2px 2px 2px #ccc', - border: `1px solid ${colors.light20}`, - borderTopLeftRadius: '4px', - borderTopRightRadius: '4px', - textAlign: 'center', -}); - -const Chevron = styled(Glyph)({ - marginRight: 4, - marginLeft: -2, - marginBottom: 1, +const MultipleSelectorSectionContainer = styled(Layout.Container)({ + padding: theme.space.medium, + background: theme.backgroundWash, + borderTop: `${theme.space.tiny}px solid ${theme.warningColor}`, + borderBottom: `${theme.space.tiny}px solid ${theme.warningColor}`, }); type MultipleSelectorSectionProps = { @@ -65,29 +40,24 @@ const MultipleSelectorSection: React.FC = memo( const [selectedId, setSelectedId] = useState( initialSelectedElement, ); - const [collapsed, setCollapsed] = useState(false); return ( - - { - setCollapsed(!collapsed); - }}> - - Multiple elements found at the target coordinates - - {!collapsed && ( - { - setSelectedId(key); - onElementSelected(key); - }} - onElementHovered={onElementHovered} - onElementExpanded={() => {}} - root={null} - selected={selectedId} - elements={elements} - /> - )} + + + Multiple elements were found at the target coordinates. Please select + one: + + { + setSelectedId(key); + onElementSelected(key); + }} + onElementHovered={onElementHovered} + onElementExpanded={() => {}} + root={null} + selected={selectedId} + elements={elements} + scrollable={false} + /> ); }, diff --git a/desktop/plugins/public/layout/__tests__/MultipleSelectionSection.node.tsx b/desktop/plugins/public/layout/__tests__/MultipleSelectionSection.node.tsx index 87ba2ced0..4b526ab43 100644 --- a/desktop/plugins/public/layout/__tests__/MultipleSelectionSection.node.tsx +++ b/desktop/plugins/public/layout/__tests__/MultipleSelectionSection.node.tsx @@ -40,30 +40,6 @@ test('rendering a single element', () => { expect(res.queryAllByText(name).length).toBe(1); }); -test('collapsing an element', () => { - const id = 'id1'; - const name = 'id_name'; - const element: Element = {...dummyElmentData, id, name}; - const res = render( - {}} - onElementHovered={null} - />, - ); - - expect(res.queryAllByText(name).length).toBe(1); - - // collapse the view - fireEvent.click(res.getByText(TITLE_STRING)); - expect(res.queryAllByText(name).length).toBe(0); - - // re-expand the view - fireEvent.click(res.getByText(TITLE_STRING)); - expect(res.queryAllByText(name).length).toBe(1); -}); - test('clicking on elements', () => { const ids = ['id1', 'id2', 'id3']; const names = ['id_name_first', 'id_name_second', 'id_name_third'];