From 5baae7437a348c5dd02dd0b5ae89911f25734c0e Mon Sep 17 00:00:00 2001 From: Cyprien P Date: Fri, 30 Apr 2021 19:28:52 +0200 Subject: [PATCH] (core) split sort and filter menu into its own button Summary: - New sort and filter button has several states - Empty / unsaved / saved - offers small save/revert button when unsaved - Fix little issue with hanging tooltip when the refElem is disposed. - The problem was that if you hover the save (or revert) button and then click the button, it causes the button to disappear, but the tooltip was staying. Test Plan: Updated all tests to match the new UI. Reviewers: paulfitz Reviewed By: paulfitz Subscribers: dsagal, paulfitz Differential Revision: https://phab.getgrist.com/D2795 --- app/client/components/ViewLayout.css | 1 + app/client/components/ViewLayout.ts | 2 +- app/client/ui/FilterBar.ts | 18 --- app/client/ui/ViewSectionMenu.ts | 220 ++++++++++++++++----------- app/client/ui/tooltips.ts | 7 + test/nbrowser/gristUtils.ts | 6 +- 6 files changed, 141 insertions(+), 113 deletions(-) diff --git a/app/client/components/ViewLayout.css b/app/client/components/ViewLayout.css index 339b2e9c..114c84a4 100644 --- a/app/client/components/ViewLayout.css +++ b/app/client/components/ViewLayout.css @@ -13,6 +13,7 @@ font-size: var(--grist-small-font-size); font-weight: 500; text-transform: uppercase; + white-space: nowrap; } .viewsection_titletext { diff --git a/app/client/components/ViewLayout.ts b/app/client/components/ViewLayout.ts index 0b23bf00..bb9134b2 100644 --- a/app/client/components/ViewLayout.ts +++ b/app/client/components/ViewLayout.ts @@ -200,7 +200,7 @@ export class ViewLayout extends DisposableWithEvents implements IDomComponent { ), dom.maybe(vs.viewInstance, (viewInstance: BaseView) => viewInstance.buildTitleControls()), dom('span.viewsection_buttons', - viewSectionMenu(this.docModel, vs, this.viewModel, this.gristDoc.isReadonly, this.gristDoc.app.useNewUI) + dom.create(viewSectionMenu, this.docModel, vs, this.viewModel, this.gristDoc.isReadonly) ) )), dom.maybe(vs.activeFilterBar, () => dom.create(filterBar, vs)), diff --git a/app/client/ui/FilterBar.ts b/app/client/ui/FilterBar.ts index 82fa13fe..fd919fd0 100644 --- a/app/client/ui/FilterBar.ts +++ b/app/client/ui/FilterBar.ts @@ -14,17 +14,6 @@ export function filterBar(_owner: IDisposableOwner, viewSection: ViewSectionRec) testId('filter-bar'), dom.forEach(viewSection.filteredFields, (field) => makeFilterField(viewSection, field, popupControls)), makePlusButton(viewSection, popupControls), - cssSpacer(), - dom.maybe(viewSection.filterSpecChanged, () => [ - primaryButton( - 'Save', testId('btn'), - dom.on('click', async () => await viewSection.saveFilters()), - ), - basicButton( - 'Revert', testId('btn'), - dom.on('click', () => viewSection.revertFilters()), - ) - ]) ); } @@ -140,16 +129,9 @@ const primaryButton = (...args: IDomArgs) => ( dom('div', cssButton.cls(''), cssButton.cls('-primary'), cssBtn.cls(''), ...args) ); -const basicButton = (...args: IDomArgs) => ( - dom('div', cssButton.cls(''), cssBtn.cls(''), ...args) -); const deleteButton = styled(primaryButton, ` padding: 3px 4px; `); -const cssSpacer = styled('div', ` - width: 8px; - flex-shrink: 0; -`); const cssPlusButton = styled(primaryButton, ` padding: 3px 3px `); diff --git a/app/client/ui/ViewSectionMenu.ts b/app/client/ui/ViewSectionMenu.ts index eeaaf9f4..2dd7cb24 100644 --- a/app/client/ui/ViewSectionMenu.ts +++ b/app/client/ui/ViewSectionMenu.ts @@ -4,88 +4,103 @@ import {CustomComputed} from 'app/client/models/modelUtil'; import {attachColumnFilterMenu} from 'app/client/ui/ColumnFilterMenu'; import {addFilterMenu} from 'app/client/ui/FilterBar'; import {makeViewLayoutMenu} from 'app/client/ui/ViewLayoutMenu'; +import {hoverTooltip} from 'app/client/ui/tooltips'; import {basicButton, primaryButton} from 'app/client/ui2018/buttons'; import {colors, vars} from 'app/client/ui2018/cssVars'; import {icon} from 'app/client/ui2018/icons'; -import {menu, menuDivider} from 'app/client/ui2018/menus'; -import {Computed, dom, fromKo, makeTestId, Observable, styled} from 'grainjs'; +import {menu} from 'app/client/ui2018/menus'; +import {Computed, dom, fromKo, IDisposableOwner, makeTestId, Observable, styled} from 'grainjs'; import difference = require('lodash/difference'); import {PopupControl} from 'popweasel'; const testId = makeTestId('test-section-menu-'); -type IconSuffix = '' | '-saved' | '-unsaved'; +const TOOLTIP_DELAY_OPEN = 750; -export function viewSectionMenu(docModel: DocModel, viewSection: ViewSectionRec, viewModel: ViewRec, - isReadonly: Observable, newUI = true) { - const emptySortFilterObs: Computed = Computed.create(null, use => { - return use(viewSection.activeSortSpec).length === 0 && use(viewSection.filteredFields).length === 0; - }); +async function doSave(docModel: DocModel, viewSection: ViewSectionRec): Promise { + await docModel.docData.bundleActions("Update Sort&Filter settings", () => Promise.all([ + viewSection.activeSortJson.save(), // Save sort + viewSection.saveFilters(), // Save filter + viewSection.activeFilterBar.save(), // Save bar + ])); +} - // Using a static subscription to emptySortFilterObs ensures that it's calculated first even if - // it started in the "unsaved" state (in which a dynamic use()-based subscription to - // emptySortFilterObs wouldn't be active, which could result in a wrong order of evaluation). - const iconSuffixObs: Computed = Computed.create(null, emptySortFilterObs, (use, empty) => { - if (use(viewSection.filterSpecChanged) || !use(viewSection.activeSortJson.isSaved) - || !use(viewSection.activeFilterBar.isSaved)) { - return '-unsaved'; - } else if (!empty) { - return '-saved'; - } else { - return ''; - } - }); +function doRevert(viewSection: ViewSectionRec) { + viewSection.activeSortJson.revert(); // Revert sort + viewSection.revertFilters(); // Revert filter + viewSection.activeFilterBar.revert(); // Revert bar +} + +export function viewSectionMenu(owner: IDisposableOwner, docModel: DocModel, viewSection: ViewSectionRec, + viewModel: ViewRec, isReadonly: Observable) { const popupControls = new WeakMap(); + const anyFilter = Computed.create(owner, (use) => Boolean(use(viewSection.filteredFields).length)); - return cssMenu( - testId('wrapper'), - dom.autoDispose(emptySortFilterObs), - dom.autoDispose(iconSuffixObs), - dom.cls(clsOldUI.className, !newUI), - dom.maybe(iconSuffixObs, () => cssFilterIconWrapper(testId('filter-icon'), cssFilterIcon('Filter'))), - cssMenu.cls(iconSuffixObs), - cssDotsIconWrapper(cssDotsIcon('Dots')), - menu(_ctl => { - return [ - dom.domComputed(use => { - use(viewSection.activeSortJson.isSaved); // Rebuild sort panel if sort gets saved. A little hacky. - return makeSortPanel(viewSection, use(viewSection.activeSortSpec), - (row: number) => docModel.columns.getRowModel(row)); - }), - dom.domComputed(viewSection.filteredFields, fields => - makeFilterPanel(viewSection, fields, popupControls)), - makeAddFilterButton(viewSection, popupControls), - makeFilterBarToggle(viewSection.activeFilterBar), - dom.domComputed(iconSuffixObs, iconSuffix => { - const displaySave = iconSuffix === '-unsaved'; - return [ + const displaySaveObs: Computed = Computed.create(owner, (use) => ( + use(viewSection.filterSpecChanged) + || !use(viewSection.activeSortJson.isSaved) + || !use(viewSection.activeFilterBar.isSaved) + )); + + const save = () => doSave(docModel, viewSection); + const revert = () => doRevert(viewSection); + + return [ + cssFilterMenuWrapper( + cssFixHeight.cls(''), + cssFilterMenuWrapper.cls('-unsaved', displaySaveObs), + testId('wrapper'), + cssMenu( + testId('sortAndFilter'), + cssFilterIconWrapper( + testId('filter-icon'), + cssFilterIconWrapper.cls('-any', anyFilter), + cssFilterIcon('Filter') + ), + menu(_ctl => [ + dom.domComputed(use => { + use(viewSection.activeSortJson.isSaved); // Rebuild sort panel if sort gets saved. A little hacky. + return makeSortPanel(viewSection, use(viewSection.activeSortSpec), + (row: number) => docModel.columns.getRowModel(row)); + }), + dom.domComputed(viewSection.filteredFields, fields => + makeFilterPanel(viewSection, fields, popupControls)), + makeAddFilterButton(viewSection, popupControls), + makeFilterBarToggle(viewSection.activeFilterBar), + dom.domComputed(displaySaveObs, displaySave => [ displaySave ? cssMenuInfoHeader( cssSaveButton('Save', testId('btn-save'), - dom.on('click', async () => { - await docModel.docData.bundleActions("Update Sort&Filter settings", () => Promise.all([ - viewSection.activeSortJson.save(), // Save sort - viewSection.saveFilters(), // Save filter - viewSection.activeFilterBar.save(), // Save bar - ])); - }), - dom.boolAttr('disabled', isReadonly), - ), + dom.on('click', save), + dom.boolAttr('disabled', isReadonly)), basicButton('Revert', testId('btn-revert'), - dom.on('click', () => { - viewSection.activeSortJson.revert(); // Revert sort - viewSection.revertFilters(); // Revert filter - viewSection.activeFilterBar.revert(); // Revert bar - }) - ) + dom.on('click', revert)) ) : null, - menuDivider() - ]; - }), - ...makeViewLayoutMenu(viewModel, viewSection, isReadonly.get()) - ]; - }) - ); + ]), + ]), + ), + dom.maybe(displaySaveObs, () => cssSaveIconsWrapper( + cssSmallIconWrapper( + cssIcon('Tick'), cssSmallIconWrapper.cls('-green'), + dom.on('click', save), + hoverTooltip(() => 'Save', {key: 'sortFilterButton', openDelay: TOOLTIP_DELAY_OPEN}), + testId('small-btn-save'), + ), + cssSmallIconWrapper( + cssIcon('CrossSmall'), cssSmallIconWrapper.cls('-gray'), + dom.on('click', revert), + hoverTooltip(() => 'Revert', {key: 'sortFilterButton', openDelay: TOOLTIP_DELAY_OPEN}), + testId('small-btn-revert'), + ), + )), + ), + cssMenu( + testId('viewLayout'), + cssFixHeight.cls(''), + cssDotsIconWrapper(cssIcon('Dots')), + menu(_ctl => makeViewLayoutMenu(viewModel, viewSection, isReadonly.get())) + ) + ]; } function makeSortPanel(section: ViewSectionRec, sortSpec: number[], getColumn: (row: number) => ColumnRec) { @@ -193,8 +208,11 @@ function makeFilterPanel(section: ViewSectionRec, filteredFields: ViewFieldRec[] const clsOldUI = styled('div', ``); -const cssMenu = styled('div', ` +const cssFixHeight = styled('div', ` margin-top: -3px; /* Section header is 24px, so need to move this up a little bit */ +`); + +const cssMenu = styled('div', ` display: inline-flex; cursor: pointer; @@ -209,19 +227,6 @@ const cssMenu = styled('div', ` &:hover, &.weasel-popup-open { background-color: ${colors.mediumGrey}; } - - &-unsaved, &-unsaved.weasel-popup-open { - border: 1px solid ${colors.lightGreen}; - background-color: ${colors.lightGreen}; - } - &-unsaved:hover { - border: 1px solid ${colors.darkGreen}; - background-color: ${colors.darkGreen}; - } - &-unsaved.${clsOldUI.className} { - border: 1px solid transparent; - background-color: ${colors.lightGreen}; - } `); const cssIconWrapper = styled('div', ` @@ -248,6 +253,20 @@ const cssMenuIconWrapper = styled(cssIconWrapper, ` } `); +const cssFilterMenuWrapper = styled('div', ` + display: inline-flex; + margin-right: 10px; + border-radius: 3px; + align-items: center; + &-unsaved { + border: 1px solid ${colors.lightGreen}; + } + & .${cssMenu.className} { + border: none; + } + +`); + const cssIcon = styled(icon, ` flex: none; cursor: pointer; @@ -272,25 +291,21 @@ const cssDotsIconWrapper = styled(cssIconWrapper, ` .${clsOldUI.className} & { border-radius: 0px; } - - .${cssMenu.className}-unsaved & { - background-color: white; - } -`); - -const cssDotsIcon = styled(cssIcon, ` - .${clsOldUI.className}.${cssMenu.className}-unsaved & { - background-color: ${colors.slate}; - } `); const cssFilterIconWrapper = styled(cssIconWrapper, ` border-radius: 2px 0px 0px 2px; + .${cssFilterMenuWrapper.className}-unsaved & { + background-color: ${colors.lightGreen}; + } `); const cssFilterIcon = styled(cssIcon, ` - .${cssMenu.className}-unsaved & { - background-color: ${colors.light}; + .${cssFilterIconWrapper.className}-any & { + background-color: ${colors.lightGreen}; + } + .${cssFilterMenuWrapper.className}-unsaved & { + background-color: white; } `); @@ -323,3 +338,26 @@ const cssMenuTextLabel = styled('span', ` const cssSaveButton = styled(primaryButton, ` margin-right: 8px; `); + +const cssSmallIconWrapper = styled('div', ` + width: 16px; + height: 16px; + border-radius: 8px; + &-green { + background-color: ${colors.lightGreen}; + } + &-gray { + background-color: ${colors.slate}; + } + & > .${cssIcon.className} { + background-color: white; + } +`); + + +const cssSaveIconsWrapper = styled('div', ` + padding: 0 6px 0 6px; + display: flex; + justify-content: space-between; + width: 54px; +`); diff --git a/app/client/ui/tooltips.ts b/app/client/ui/tooltips.ts index 21c824c1..79f74bee 100644 --- a/app/client/ui/tooltips.ts +++ b/app/client/ui/tooltips.ts @@ -74,12 +74,15 @@ export function showTooltip( ): ITooltipControl { const placement: Popper.Placement = options.placement || 'top'; const key = options.key; + let closed = false; // If we had a previous tooltip with the same key, clean it up. if (key) { openTooltips.get(key)?.close(); } // Cleanup involves destroying the Popper instance, removing the element, etc. function close() { + if (closed) { return; } + closed = true; popper.destroy(); dom.domDispose(content); content.remove(); @@ -98,6 +101,9 @@ export function showTooltip( }; const popper = new Popper(refElem, content, popperOptions); + // If refElem is disposed we close the tooltip. + dom.onDisposeElem(refElem, close); + // Fade in the content using transitions. prepareForTransition(content, () => { content.style.opacity = '0'; }); content.style.opacity = ''; @@ -142,6 +148,7 @@ export function setHoverTooltip(refElem: Element, tipContent: ITooltipContentFun tipControl = showTooltip(refElem, ctl => tipContent({...ctl, close}), options); dom.onElem(tipControl.getDom(), 'mouseenter', clearTimer); dom.onElem(tipControl.getDom(), 'mouseleave', scheduleCloseIfOpen); + dom.onDisposeElem(tipControl.getDom(), close); if (timeoutMs) { resetTimer(close, timeoutMs); } } function close() { diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index b7cae450..957d2d41 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -884,7 +884,7 @@ export async function toggleFilterBar(goal: 'open'|'close'|'toggle' = 'toggle', (goal === 'open') && isOpen ) { return; } - const menu = await openSectionMenu(options.section); + const menu = await openSectionMenu('sortAndFilter', options.section); await menu.findContent('.grist-floating-menu > div', /Toggle Filter Bar/).find('.test-section-menu-btn').click(); if (options.save) { await menu.findContent('.grist-floating-menu button', /Save/).click(); @@ -896,9 +896,9 @@ export async function toggleFilterBar(goal: 'open'|'close'|'toggle' = 'toggle', /** * Opens the section menu for a section, or the active section if no section is given. */ -export async function openSectionMenu(section?: string|WebElement) { +export async function openSectionMenu(which: 'sortAndFilter'|'viewLayout', section?: string|WebElement) { const sectionElem = section ? await getSection(section) : await driver.findWait('.active_section', 4000); - await sectionElem.find('.test-section-menu-wrapper').click(); + await sectionElem.find(`.test-section-menu-${which}`).click(); return await driver.findWait('.grist-floating-menu', 100); }