(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

@@ -13,6 +13,7 @@
   font-size: var(--grist-small-font-size);
   font-weight: 500;
   text-transform: uppercase;
+  white-space: nowrap;
 }
 
 .viewsection_titletext {

app/client/components/ViewLayout.ts

@@ -200,7 +200,7 @@ export class ViewLayout extends DisposableWithEvents implements IDomComponent {
         ),
         dom.maybe<BaseView|null>(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)),

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<HTMLDivElement>) => (
   dom('div', cssButton.cls(''), cssButton.cls('-primary'),
       cssBtn.cls(''), ...args)
 );
-const basicButton = (...args: IDomArgs<HTMLDivElement>) => (
-  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
 `);

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 {menu} from 'app/client/ui2018/menus';
-import {Computed, dom, fromKo, makeTestId, Observable, styled} from 'grainjs';
+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,
+async function doSave(docModel: DocModel, viewSection: ViewSectionRec): Promise<void> {
-                                isReadonly: Observable<boolean>, newUI = true) {
+  await docModel.docData.bundleActions("Update Sort&Filter settings", () => Promise.all([
-  const emptySortFilterObs: Computed<boolean> = Computed.create(null, use => {
+    viewSection.activeSortJson.save(),  // Save sort
-    return use(viewSection.activeSortSpec).length === 0 && use(viewSection.filteredFields).length === 0;
+    viewSection.saveFilters(),          // Save filter
-  });
+    viewSection.activeFilterBar.save(), // Save bar
+  ]));
+}
 
-  // Using a static subscription to emptySortFilterObs ensures that it's calculated first even if
+function doRevert(viewSection: ViewSectionRec) {
-  // it started in the "unsaved" state (in which a dynamic use()-based subscription to
+  viewSection.activeSortJson.revert();  // Revert sort
-  // emptySortFilterObs wouldn't be active, which could result in a wrong order of evaluation).
+  viewSection.revertFilters();          // Revert filter
-  const iconSuffixObs: Computed<IconSuffix> = Computed.create(null, emptySortFilterObs, (use, empty) => {
+  viewSection.activeFilterBar.revert(); // Revert bar
-    if (use(viewSection.filterSpecChanged) || !use(viewSection.activeSortJson.isSaved)
+}
-        || !use(viewSection.activeFilterBar.isSaved)) {
+
-      return '-unsaved';
+export function viewSectionMenu(owner: IDisposableOwner, docModel: DocModel, viewSection: ViewSectionRec,
-    } else if (!empty) {
+                                viewModel: ViewRec, isReadonly: Observable<boolean>) {
-      return '-saved';
-    } else {
-      return '';
-    }
-  });
 
   const popupControls = new WeakMap<ViewFieldRec, PopupControl>();
+  const anyFilter = Computed.create(owner, (use) => Boolean(use(viewSection.filteredFields).length));
 
-  return cssMenu(
+  const displaySaveObs: Computed<boolean> = Computed.create(owner, (use) => (
-    testId('wrapper'),
+    use(viewSection.filterSpecChanged)
-    dom.autoDispose(emptySortFilterObs),
+      || !use(viewSection.activeSortJson.isSaved)
-    dom.autoDispose(iconSuffixObs),
+      || !use(viewSection.activeFilterBar.isSaved)
-    dom.cls(clsOldUI.className, !newUI),
+  ));
-    dom.maybe(iconSuffixObs, () => cssFilterIconWrapper(testId('filter-icon'), cssFilterIcon('Filter'))),
+
-    cssMenu.cls(iconSuffixObs),
+  const save = () => doSave(docModel, viewSection);
-    cssDotsIconWrapper(cssDotsIcon('Dots')),
+  const revert = () => doRevert(viewSection);
-    menu(_ctl => {
+
-      return [
+  return [
-        dom.domComputed(use => {
+    cssFilterMenuWrapper(
-          use(viewSection.activeSortJson.isSaved); // Rebuild sort panel if sort gets saved. A little hacky.
+      cssFixHeight.cls(''),
-          return makeSortPanel(viewSection, use(viewSection.activeSortSpec),
+      cssFilterMenuWrapper.cls('-unsaved', displaySaveObs),
-                               (row: number) => docModel.columns.getRowModel(row));
+      testId('wrapper'),
-        }),
+      cssMenu(
-        dom.domComputed(viewSection.filteredFields, fields =>
+        testId('sortAndFilter'),
-          makeFilterPanel(viewSection, fields, popupControls)),
+        cssFilterIconWrapper(
-        makeAddFilterButton(viewSection, popupControls),
+          testId('filter-icon'),
-        makeFilterBarToggle(viewSection.activeFilterBar),
+          cssFilterIconWrapper.cls('-any', anyFilter),
-        dom.domComputed(iconSuffixObs, iconSuffix => {
+          cssFilterIcon('Filter')
-          const displaySave = iconSuffix === '-unsaved';
+        ),
-          return [
+        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 () => {
+                            dom.on('click', save),
-                  await docModel.docData.bundleActions("Update Sort&Filter settings", () => Promise.all([
+                            dom.boolAttr('disabled', isReadonly)),
-                    viewSection.activeSortJson.save(),  // Save sort
-                    viewSection.saveFilters(),          // Save filter
-                    viewSection.activeFilterBar.save(), // Save bar
-                  ]));
-                }),
-                dom.boolAttr('disabled', isReadonly),
-              ),
               basicButton('Revert', testId('btn-revert'),
-                dom.on('click', () => {
+                          dom.on('click', revert))
-                  viewSection.activeSortJson.revert();  // Revert sort
-                  viewSection.revertFilters();          // Revert filter
-                  viewSection.activeFilterBar.revert(); // Revert bar
-                })
-              )
             ) : 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 & {
+  .${cssFilterIconWrapper.className}-any & {
-    background-color: ${colors.light};
+    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;
+`);

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() {

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);
 }