Dialog for hidden filters [issue-78] #90

JaySunSyn · 2017-11-17T16:07:59Z

Please merge #117 first

plequang · 2017-11-20T09:50:55Z

cosmoz-omnitable.html

@@ -52,7 +54,15 @@
 					<paper-checkbox checked$="{{ _allSelected }}" on-change="_onAllCheckboxChange" hidden$="[[ !_dataIsValid ]]">
 					</paper-checkbox>
 				</div>
-				<cosmoz-omnitable-header-row columns="[[ visibleColumns ]]" group-on-column="[[ groupOnColumn ]]"></cosmoz-omnitable-header-row>
+				<cosmoz-omnitable-header-row columns="[[ visibleColumns ]]" group-on-column="[[ groupOnColumn ]]">
+					<div slot="suffix" class="align-right" hidden$="[[allVisible]]">


This breaks _adjustHeadersWidth.
In _adjustHeadersWidth, we reference individual header cell by their index in header row children (see https://github.com/Neovici/cosmoz-omnitable/blob/master/cosmoz-omnitable.js#L786)
But now, the suffix div is the first children in header row, so we don't size the right element anymore

Thanks, pls let me know if the changes work for you.

plequang · 2017-11-20T09:59:11Z

cosmoz-omnitable-filter-dialog.html

+    		is: 'cosmoz-omnitable-filter-dialog',
+
+    		behaviors: [
+    			Polymer.PaperDialogBehavior,


We can't use paper-dialog, because it has stacking context issues (the dialog can appear below another element).
cosmoz-dialog solves stacking context issues, but works differently.
Instead, you should use

<cosmoz-dialog> <template> dialog content <template>

Regarding the dialog content, you should investigate using cosmoz-omnitable-repeater-behavior, as this behavior already handles columns changes and instances creation/release.
See for example https://github.com/Neovici/cosmoz-omnitable/blob/master/cosmoz-omnitable-header-row.html

plequang · 2017-11-20T10:02:27Z

cosmoz-omnitable.html

@@ -195,7 +205,13 @@ <h3 class="groupRow-label">
 		<div id="columns">
 			<slot></slot>
 		</div>
-
+
+		<cosmoz-omnitable-filter-dialog


So this should rather looks like (relating to other comments):

<cosmoz-dialog with-backdrop> <template> <cosmoz-omnitable-filters-dialog columns="[[disabledColumns]]"> <template>

plequang

See code comments

plequang · 2017-11-22T21:10:52Z

cosmoz-omnitable.js

@@ -297,6 +304,14 @@
 			return dataIsValid && hasActions;
 		},

+		_computeColumnsAllVisible(visibleColumnsChange, columns) {


This does not work for the groupOn column.
When grouping on a column, it is not removed from the visibleColumn property, but the cells for the groupOn colunm are hidden.
The _computeColumnsAllVisible function could also observe changes to the groupOnColumn property, and If this is not null, then it should return false

plequang · 2017-11-22T21:15:58Z

cosmoz-omnitable.html

@@ -196,6 +206,16 @@ <h3 class="groupRow-label">
 			<slot></slot>
 		</div>

+		<cosmoz-dialog with-backdrop id="filterDialog">


I think the dialog needs at least a "Close" button.
It's not always clear to the user that clicking somewhere outside the dialog will close it.

plequang · 2017-11-22T21:33:05Z

cosmoz-omnitable-headers-dialog.html

+					return;
+				}
+
+				headers.innerHTML = '';


This is not the right way to clear the content of the dialog.
cosmoz-omnitable-templatizer keeps a reference to the instances it creates, so when an instance is not used anymore it needs to be "released", otherwise it will stay in memory.
This is also not very efficient, as we recreate new instances for all headers in the dialog when disabledColumns is changing.

You should really try to use cosmoz-omnitable-repeater-behavior, as it takes care of correctly releasing unused instances as well as creating only instances when needed.
cosmoz-omnitable-headers-dialog could be implemented the same way as cosmoz-omnitable-header-row.

JaySunSyn · 2017-11-29T11:30:01Z

Please check again.

plequang · 2017-11-29T14:32:32Z

cosmoz-omnitable.js

+			 */
+			allColumnsVisible: {
+				type: Boolean,
+				computed: '_computeColumnsAllVisible(visibleColumns.*, groupOnColumn.*, columns)'


This won't work correctly when groupOnColumn is undefined because complex observers are called only when all properties are defined.
So when groupOn is not defined, groupOnColumn is undefined and the computed function is not called.

Currently, the open filters dialog icon is always visible.

plequang · 2017-11-29T14:35:39Z

cosmoz-omnitable.js

 			}
 		},

 		observers: [
 			'_dataChanged(data.*)',
 			'_debounceSortItems(sortOn, descending, filteredGroupedItems)',
 			'_routeHashParamsChanged(_routeHashParams.*, hashParam, columns)',
-			' _selectedItemsChanged(selectedItems.*)'
+			'_selectedItemsChanged(selectedItems.*)',
+			'_setFilterDialogColumns(disabledColumns.*, groupOnColumn.*)'


Same as for _computeColumnsAllVisible.
_setFilterDialogColumns will never be called as long as there is no group on column

plequang · 2017-11-29T14:57:56Z

cosmoz-omnitable-headers-dialog.html

+			}
+		</style>
+
+		<paper-icon-button class="close-btn" icon="close" on-tap="_close"></paper-icon-button>


It does not feel natural to have to click top right.
I think a button inside the dialog, bottom right, would be better. Like in paper-dialog demos.
Not sure about the label though.
Let's use "Close" for now.
So the close button should move the cosmoz-dialog template in omnitable.html, a <div class="buttons> to use paper-dialog default styling.

Hmm, took the facebook approach here.. But I thinks you are right..

plequang · 2017-11-29T15:02:45Z

cosmoz-omnitable-headers-dialog.html

+-->
+<dom-module id="cosmoz-omnitable-headers-dialog">
+	<template>
+		<style>


Styling of the dialog looks strange.
host might require a display block.
container should use a vertical flexbox.

plequang · 2017-11-29T15:05:47Z

cosmoz-omnitable-headers-dialog.html

+		<paper-icon-button class="close-btn" icon="close" on-tap="_close"></paper-icon-button>
+
+		<div class="container">
+			<div class="title">[[title]]</div>


Remove the title from here, and use <h2>...</h2> in the dialog template in omnitable.html
That way, it will use paper-dialog default styling.

plequang · 2017-11-29T15:07:30Z

cosmoz-omnitable.html

@@ -196,6 +208,17 @@ <h3 class="groupRow-label">
 			<slot></slot>
 		</div>

+		<cosmoz-dialog with-backdrop id="filterDialog">
+			<template>


<h2>[[_('Filter', t)]]</h2>

plequang · 2017-11-29T15:08:20Z

cosmoz-omnitable.html

+					columns="[[_filterDialogColumns]]"
+					on-close-tapped="_closeFilterDialog"
+					with-backdrop>
+				</cosmoz-omnitable-headers-dialog>


<div class="buttons> <paper-button>[[_("Close")]]</paper-button> </div>

plequang

_close() function in headers-dialog not needed anymore ?
Thanks to this PR, we don't have to reset filters when groupon column changes (this was some sort of hack). See https://github.com/Neovici/cosmoz-omnitable/blob/master/cosmoz-omnitable.js#L536-L538

Code looks good, and the dialog works as expected.

Some UI/UX issues have to be fixed, though I'm not sure how:

If the dialog contains only an autocomplete header, interaction with the dropdown is not possible (try grouping on "Group" column).
The "filter' icon to open the dialog is overlaying the last header (not always. Try to group on Amount in the demo).

plequang · 2017-12-03T21:11:52Z

cosmoz-omnitable-headers-dialog.html

+				this._forwardPropertiesFlush(instance);
+			},
+
+			_close() {


Since the close button is now in omnitable.html, is this function still needed ?

plequang · 2017-12-03T21:44:14Z

Some screenshot to illustrate the UI/UX issues:

Single filter with paper-autocomplete-chips:

Icon overlaying header:

Even more overlaying icon, when there is no scrollbar in the omnitable

JaySunSyn · 2017-12-08T11:36:32Z

The not-overlapping suggestions of autocomplete-chips is fixed with this PR Neovici/paper-autocomplete-chips#6

JaySunSyn · 2017-12-19T17:01:22Z

Hmm :( sorry for that - I cannot reproduce.. May you check again if you have integrated the PR Neovici/paper-autocomplete-chips#6 and pull also this PR again please. Thanks

plequang · 2017-12-19T20:47:25Z

OK my bad. I was using bower link paper-autocomplete-chips, which links to bower_components directory, and not the bower_components-1.x directory.

plequang · 2017-12-19T21:14:15Z

Found an issue. To reproduce, follow these steps in the full demo:

Reduce the width of the screen to let the filter icon appear
open the filter dialog, the close it
reduce more the width, so that new filters are added to the dialog.
open the dialog

=> the paper-autocomplete will have a width of 0
When the dialog is opened for the first time, _onNodesChange is called while the dialog is visible, so offsetWidth returns something different than 0
When you reduce the width at step 3, the dialog is not visible, so in _onNodesChange, offsetWidth will return 0.

plequang · 2017-12-19T21:16:02Z

Found another issue, not related to paper-autocomplete:

reduce the width of the screen to let the filter icon appear
open the filter dialog
maximize the window

=> the dialog is displayed without any filter

…sChange

needed for clear btn

plequang · 2017-12-21T21:17:37Z

cosmoz-omnitable-headers-dialog.html

+			},
+
+			_onResize() {
+				this.scopeSubtree(this.$.container);


What's the purpose of this ?

Actually, this seems to break styling of the suggestion wrapper, which does not overlap anymore (but it works if I remove the scopeSubtree call)

Hmm :/ yes, I see this too now..
It's to apply the styles of .header-clear-filter to some intputs

Without:

But it seems that we should not use scopeSubtree for polymer elements. Only for external libs..

Looking into it. Trying to find a way without putting the style into the template

JaySunSyn · 2017-12-29T13:35:02Z

Added a clear button in PR #117 to apply styles correctly.

* Added clear button element * Use clear button instead of iron-icon Currently needed to display the clear button in the filters-dialog (PR #90) without having to include the styles into the template.

plequang

Found another issue, with autocomplete columns.
To reproduce in full-demo.html, update x-page to set priority="-1" to the group column.
Then reduce the window width so that the group column is hidden. Open the filter dialog, and click in the group filter. The dropdown should open with possible group values.
Close the filter dialog.
Increase window size so that the group column is visible again.
Click in the Group filter => the available values dropdown does not show.

Solved issue, with autocomplete columns. To reproduce in full-demo.html, update x-page to set priority="-1" to the group column. Then reduce the window width so that the group column is hidden. Open the filter dialog, and click in the group filter. The dropdown should open with possible group values. Close the filter dialog. Increase window size so that the group column is visible again. Click in the Group filter => the available values dropdown does not show.

JaySunSyn · 2018-01-12T13:33:30Z

Please merge #135 first so building succeeds again.

plequang

Found another issue.

To reproduce in full-demo, group the table on the last column (Amount). The dialog filter icon does not appear, because we don't want to filter on the group on column.
Then reduce the window width to make the icon appear.
Open the dialog => Amount filter is visible, but it shouldn't.

into issue-78

Calling done() in an eventlistener => ensure that it will be called only once

plequang

Found an issue when using cosmoz-omnitable-treenode-column : for this column type, the header is using paper-autocomplete, and the dropdown causing the dialog to scroll.
Actually, same issue as before with paper-autocomplete.
You can try it in treenode column repository, and using bower link cosmoz-omnitable.
Don't know if this can be solved in omnitable or in treenode-column.

JaySunSyn added 3 commits November 17, 2017 15:37

proof of concept

7870516

Implemented with paper-dialog-behavior

dd4974b

Reverted changes column-amount

f6e3b4f

plequang requested changes Nov 20, 2017

View reviewed changes

JaySunSyn added 5 commits November 21, 2017 09:26

Integrated with cosmoz-dialog

36f3e4f

Fixed _adjustHeadersWidth

0a48825

Cleanup & docs

3a743ab

new line fix

c075188

small _adjustHeadersWidth enhancment

f1aedc3

plequang requested changes Nov 22, 2017

View reviewed changes

JaySunSyn added 7 commits November 28, 2017 10:56

Added close button

5d268db

remove children

6196ebc

Replaced with OmnitableRepeaterBehavior

8c099a7

Removed suffix slot of header-row

c76ee43

Sorted header columns

90f3fe2

fixed codeclimate issue

d6c3018

fixed hide if all colums are visible

02018f4

plequang requested changes Nov 29, 2017

View reviewed changes

JaySunSyn added 5 commits November 30, 2017 17:05

PR comments

fc7fda6

minor fix

98680ed

Merge branch 'master' into issue-78

3a0810b

whitespace fix

a7c9fd9

Minor style improvments

7c26f3e

plequang requested changes Dec 3, 2017

View reviewed changes

JaySunSyn added 2 commits December 8, 2017 12:32

Fixed filter btn position

4d8e263

PR comment

ea93d18

PR comment

27cb2d4

JaySunSyn added 3 commits December 20, 2017 17:29

Set width of paper-autocomplete-suggestions _onResize instead _onNode…

0ddec4a

…sChange

close filterdialog if no _filterDialogColumns

bf02f2d

scopeSubtree to apply existing header styles

59d92d3

needed for clear btn

plequang requested changes Dec 21, 2017

View reviewed changes

JaySunSyn mentioned this pull request Dec 29, 2017

Clear button element instead of iron-icon with class #117

Merged

added debouncer

1267167

minor style update

58a450b

plequang requested changes Jan 10, 2018

View reviewed changes

JaySunSyn added 3 commits January 11, 2018 20:39

Merge remote-tracking branch 'upstream/master' into issue-78

49d6f6f

Merge remote-tracking branch 'upstream/master' into issue-78

369e202

Merge branch 'master' into issue-78

e2a8e7c

plequang requested changes Jan 12, 2018

View reviewed changes

JaySunSyn added 6 commits January 16, 2018 11:43

Merge branch 'issue-78' of https://github.com/JaySunSyn/cosmoz-omnitable

6dc1d6f

into issue-78

Merge remote-tracking branch 'upstream/master' into issue-78

5406848

fixed filterDialogColumns

4b827f2

Merge remote-tracking branch 'upstream/master' into issue-78

a3485ff

Merge remote-tracking branch 'upstream/master' into issue-78

9cd4c92

fixed tests

17adf74

Calling done() in an eventlistener => ensure that it will be called only once

plequang requested changes Jan 17, 2018

View reviewed changes

JaySunSyn added 2 commits January 19, 2018 11:51

Handle paper-autocomplete headers

8d6c20f

Ensure wrapper sizing after resize

e1d5bfa

megheaiulian closed this Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dialog for hidden filters [issue-78] #90

Dialog for hidden filters [issue-78] #90

JaySunSyn commented Nov 17, 2017 •

edited

Loading

plequang Nov 20, 2017

JaySunSyn Nov 21, 2017

plequang Nov 20, 2017

plequang Nov 20, 2017

plequang left a comment

plequang Nov 22, 2017

plequang Nov 22, 2017

plequang Nov 22, 2017

JaySunSyn commented Nov 29, 2017

plequang Nov 29, 2017

plequang Nov 29, 2017

plequang Nov 29, 2017

JaySunSyn Nov 30, 2017

plequang Nov 29, 2017

plequang Nov 29, 2017

plequang Nov 29, 2017

plequang Nov 29, 2017

plequang left a comment

plequang Dec 3, 2017

plequang commented Dec 3, 2017

JaySunSyn commented Dec 8, 2017

JaySunSyn commented Dec 19, 2017 •

edited

Loading

plequang commented Dec 19, 2017

plequang commented Dec 19, 2017

plequang commented Dec 19, 2017

plequang Dec 21, 2017

JaySunSyn Dec 23, 2017 •

edited

Loading

JaySunSyn commented Dec 29, 2017

plequang left a comment

JaySunSyn commented Jan 12, 2018

plequang left a comment

plequang left a comment

Dialog for hidden filters [issue-78] #90

Dialog for hidden filters [issue-78] #90

Conversation

JaySunSyn commented Nov 17, 2017 • 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

plequang left a 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

JaySunSyn commented Nov 29, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plequang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plequang commented Dec 3, 2017

JaySunSyn commented Dec 8, 2017

JaySunSyn commented Dec 19, 2017 • edited Loading

plequang commented Dec 19, 2017

plequang commented Dec 19, 2017

plequang commented Dec 19, 2017

Choose a reason for hiding this comment

JaySunSyn Dec 23, 2017 • edited Loading

Choose a reason for hiding this comment

JaySunSyn commented Dec 29, 2017

plequang left a comment

Choose a reason for hiding this comment

JaySunSyn commented Jan 12, 2018

plequang left a comment

Choose a reason for hiding this comment

plequang left a comment

Choose a reason for hiding this comment

JaySunSyn commented Nov 17, 2017 •

edited

Loading

JaySunSyn commented Dec 19, 2017 •

edited

Loading

JaySunSyn Dec 23, 2017 •

edited

Loading