feat: add load more pager to filters #3760

GZolla · 2023-06-20T16:46:34Z

Adding load more pager to filters. Added the pager-has-more property to d2l-filter-dimension-set that when set will render a d2l-pager-load-more button (rendering done in d2l-filter).
The handling of pagination and adding/removing the property from the dimension is still the responsibility of the consumer (see the added demo as an example).

Rally: US153373

github-actions · 2023-06-20T16:48:13Z

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL	https://pr-3760-brightspace-ui-core.d2l.dev/

Note
The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

dbatiste · 2023-06-21T19:11:35Z

components/filter/filter.js

+		const dimensionKey = e.target.parentNode.id.slice(SET_DIMENSION_ID_PREFIX.length);
+
+		/** @ignore */
+		this.dispatchEvent(new CustomEvent('d2l-filter-dimension-load-more', {


Would a consumer ever try to do this?

<d2l-filter-dimension-set @d2l-filter-dimension-load-more=${...}>...</d2l-filter-dimension-set>

Are we sure the <filter> is the correct element to dispatch the event?

Consumers would need to check for the key in the event. I replicated the behavior for the d2l-filter-dimension-empty-state-action, since rendering happens on filter it is also responsible for dispatching events.

Why are we /** @ignore */-ing the event?

Yeah having the filter dispatch all the events is a side-effect of our choice to have filter render everything, which we recently met about and decided not to change, even with the limitations. I think we could technically dispatch the event off the corresponding data-only dimension component, but like @GZolla mentioned the other dimension-specific events currently don't work this way. I'm also not sure if the analyzer for the Daylight site would understand where to put the event in that case?

svanherk · 2023-06-23T13:42:29Z

components/filter/demo/filter-load-more-demo.js

+
+		return html`<d2l-filter-dimension-set key="${dimension.key}" text="${dimension.text}" ?pager-has-more="${loadedItems.length < items.length}">
+			${repeat(loadedItems, item => item.key, item => html`
+				<d2l-filter-dimension-set-value key="${item.key}" text="${item.text}"></d2l-filter-dimension-set-value>


Looks like loading more items unselects everything, which I suspect is just be a demo problem. I think you'd need to handle tracking and setting the selected state like this demo does. Which I think is worth it, to give people a realistic example.

The last commit modifies the demo to handle this

components/filter/filter.js

svanherk · 2023-06-23T13:48:50Z

components/filter/filter.js

+		const dimensionKey = e.target.parentNode.id.slice(SET_DIMENSION_ID_PREFIX.length);
+
+		/** @ignore */
+		this.dispatchEvent(new CustomEvent('d2l-filter-dimension-load-more', {


Why are we /** @ignore */-ing the event?

svanherk · 2023-06-23T13:57:16Z

components/filter/demo/filter-load-more-demo.js

So this is probably a @geurts question, but one thing this doesn't handle at all is selected items that haven't been loaded. If the filter is pre-populated with some selected things (like in Quick Eval where filters are re-applied when hitting the back button), and those items aren't included in the initial load, all the counts we show will be wrong because we expect to have all options.

The demo now uses the same logic we use for search: we add all items but hide them and then show only those that are "loaded". This means the count will be correct even when some values are hidden by the search or the load more.

svanherk · 2023-06-23T13:59:15Z

components/filter/demo/filter-load-more-demo.js

Also, if a consumer is using load more they'll have to be using manual search, we don't have all the values to search for them. Should we enforce that somehow, like ignore the search value set and pass manual if has-more is set? Maybe another @geurts question

I'm not sure what the best approach is here, but looking at other components we could just leave it as is. If manual search is not set the load more will work but the search would just filter the added items which sort of breaks its use. To me this is similar to failing to add keys to list items, we don't generate a key but just let the component break, meaning that the consumer is solely responsible for properly setting up the component.

components/filter/filter.js

github-actions · 2023-06-27T20:07:22Z

Visual diff tests failed - pull request #3788 has been opened with the updated goldens.

Co-authored-by: github-actions <[email protected]>

@ignore

- Remove /** @ignore */ from load more event and add documentation - Fix demo resetting selection - Modify the complete callback to take the keys to be loaded

…re into gzolla/filter-load-more

svanherk · 2023-06-28T18:18:24Z

components/filter/filter-dimension-set.js

+			 * Wether the dimension has more values to load
+			 * @type {boolean}
+			 */
+			pagerHasMore: { type: Boolean, attribute: 'pager-has-more', reflect: true },


Suggested change

pagerHasMore: { type: Boolean, attribute: 'pager-has-more', reflect: true },

hasMore: { type: Boolean, attribute: 'has-more', reflect: true },

I wonder if just has-more would be better here, since a consumer doesn't really know we're using pager under the hood

I still like this being named has-more to avoid revealing implementation details.

svanherk · 2023-06-28T18:18:28Z

components/filter/demo/filter-load-more-demo.js

+					{ key: 'chemistry', selected:false, text: 'Chemistry' },
+					{ key: 'drama', selected:false, text: 'Drama' },
+					{ key: 'english', selected:false, text: 'English' },
+					{ key: 'how-t selected:false,o', text: 'How To Write a How To Article With a Flashy Title' },


Suggested change

{ key: 'how-t selected:false,o', text: 'How To Write a How To Article With a Flashy Title' },

{ key: 'how-to', selected:false, text: 'How To Write a How To Article With a Flashy Title' },

components/filter/filter.js

components/filter/demo/filter-load-more-demo.js

svanherk · 2023-07-11T22:46:35Z

components/filter/filter-dimension-set.js

+			 * Wether the dimension has more values to load
+			 * @type {boolean}
+			 */
+			pagerHasMore: { type: Boolean, attribute: 'pager-has-more', reflect: true },


I still like this being named has-more to avoid revealing implementation details.

components/filter/filter.js

svanherk · 2023-07-13T14:16:37Z

components/filter/filter-dimension-set.js

@@ -45,7 +50,7 @@ class FilterDimensionSet extends LitElement {
 			 * Whether to render the selected items at the top of the filter
 			 * @type {boolean}
 			 */
-			selectedFirst: { type: Boolean, attribute: 'selected-first' },
+			selectedFirst: { type: Boolean, attribute: 'selected-first', reflect : true },


Is the reflect needed to work, or is it just to tell the consumer we changed the value on them?

svanherk · 2023-07-13T14:23:37Z

components/filter/filter-dimension-set.js

+	willUpdate(changedProperties) {
+		if (changedProperties.has('hasMore') && this.hasMore) {
+			if (this.searchType !== 'manual') {
+				console.warn('Paging requires search type set to manual.');
+				this.hasMore = false;
+			}
+			else this.selectedFirst = true;
+		}
+	}


Interested in some other opinions here. Currently, if you've set has-more but not search-type="manual", we turn off has-more and don't render the pager. We can't just turn on manual search for them, because they won't have events setup to handle it. Then, if they have correctly configured search, we override their setting of selected-first because we also require that to be on (and we can force that on for them, there's nothing they need to do).

This fails pretty gracefully, but just wondering if we should actually be more destructive to make it obvious things are misconfiguring? Should we log a console error in one or both scenarios, and/or be less graceful and more obviously broken?

I'm good with this, but wanted to check since we've used a few different strategies across core before

svanherk · 2023-07-13T14:37:13Z

components/filter/filter.js

+				if (dimension.selectedFirst) {
+					this._updateDimensionShouldBubble(dimension);
+				}


Are we now running this twice for the search scenario (once here, and then again above)?

If so, could we move it into _performDimensionSearch? Seems like that hits all paths - automatic search, manual search, and search after load more?

svanherk · 2023-07-13T14:38:01Z

components/filter/filter.js

+		this.dispatchEvent(new CustomEvent('d2l-filter-dimension-load-more', {
+			detail: {
+				key: dimensionKey,
+				value: dimension.searchValue,


Maybe searchValue to make it clear, or currentlyAppliedSearch?

My thought process was to have the same attribute names between this and the manual search event. Since we also decided to document those together, I think it makes it more intuitive to the consumer.

svanherk · 2023-07-13T14:38:48Z

components/list/test/screenshots/ci/golden/list/d2l-list-nested-all-selected.png

Do you know why this visual diff is here? I wonder if merging main in would remove this, doesn't seem related to your changes

svanherk

Just realized we should probably add a visual-diff test that shows the load-more pager, and has a couple values selected-first but selected-first isn't applied. But I'm fine with that being a follow-up PR!

svanherk · 2023-07-14T13:02:55Z

components/filter/filter-dimension-set.js

@@ -31,6 +31,11 @@ class FilterDimensionSet extends LitElement {
 			 * @type {boolean}
 			 */
 			loading: { type: Boolean },
+			/**
+			 * Wether the dimension has more values to load


Suggested change

* Wether the dimension has more values to load

* Whether the dimension has more values to load. Must be used with selected-first and search-type="manual"

Could also be done as part of the documentation story if you're trying to get this merged before branching!

svanherk · 2023-07-14T13:14:32Z

Also, just realized the focus is being lost after hitting "Load More". @dbatiste, is this something the list is supposed to handle automatically, or the consumer has to?

dbatiste · 2023-07-14T15:23:23Z

Also, just realized the focus is being lost after hitting "Load More". @dbatiste, is this something the list is supposed to handle automatically, or the consumer has to?

I don't think focus should be lost. There were some previous changes that would disable the button while loading, and that did cause focus to be lost, but that was removed a long time ago. Focus should stay on the button while loading, and when the consumer calls e.detail.complete() we focus the first newly loaded item (based on index). We do ignore subsequent clicks while loading.

ghost · 2023-07-14T20:10:05Z

🎉 This PR is included in version 2.134.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

svanherk · 2023-07-17T19:31:20Z

Also, just realized the focus is being lost after hitting "Load More". @dbatiste, is this something the list is supposed to handle automatically, or the consumer has to?

I don't think focus should be lost. There were some previous changes that would disable the button while loading, and that did cause focus to be lost, but that was removed a long time ago. Focus should stay on the button while loading, and when the consumer calls e.detail.complete() we focus the first newly loaded item (based on index). We do ignore subsequent clicks while loading.

@GZolla Is there a defect or something to track this? Just don't want to forget about it

feat: add load more pager to filters

876dfdc

GZolla marked this pull request as ready for review June 20, 2023 16:52

GZolla requested a review from a team as a code owner June 20, 2023 16:52

dbatiste reviewed Jun 21, 2023

View reviewed changes

svanherk reviewed Jun 23, 2023

View reviewed changes

Merge branch 'main' into gzolla/filter-load-more

6c4eae4

github-actions bot mentioned this pull request Jun 27, 2023

Updating Visual Diff Goldens for PR 3760 #3788

Merged

github-actions bot and others added 3 commits June 27, 2023 16:09

Updating Visual Diff Goldens (#3788)

b3986d2

Co-authored-by: github-actions <[email protected]>

fixes for filter load more

d7c3e90

- Remove /** @ignore */ from load more event and add documentation - Fix demo resetting selection - Modify the complete callback to take the keys to be loaded

Merge branch 'gzolla/filter-load-more' of github.com:BrightspaceUI/co…

748b408

…re into gzolla/filter-load-more

svanherk reviewed Jun 28, 2023

View reviewed changes

improving demo

49dd176

svanherk reviewed Jul 11, 2023

View reviewed changes

GZolla added 2 commits July 12, 2023 12:39

fixes from review

6a7416f

maintain name consistency in search/paging event

518f5c4

svanherk reviewed Jul 13, 2023

View reviewed changes

GZolla added 2 commits July 13, 2023 10:49

Merge branch 'main' into gzolla/filter-load-more

59289c9

remove reflect, refactor selectedFirst call

1b116d6

svanherk approved these changes Jul 14, 2023

View reviewed changes

await update to prevent focus being lost on load more

06fb924

GZolla merged commit efc6310 into main Jul 14, 2023

GZolla deleted the gzolla/filter-load-more branch July 14, 2023 20:04

ghost added the released label Jul 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add load more pager to filters #3760

feat: add load more pager to filters #3760

GZolla commented Jun 20, 2023

github-actions bot commented Jun 20, 2023

dbatiste Jun 21, 2023 •

edited

Loading

GZolla Jun 21, 2023

svanherk Jun 23, 2023

svanherk Jun 27, 2023

svanherk Jun 23, 2023

GZolla Jun 27, 2023

svanherk Jun 23, 2023

svanherk Jun 23, 2023

GZolla Jun 27, 2023

svanherk Jun 23, 2023

GZolla Jun 27, 2023

github-actions bot commented Jun 27, 2023

svanherk Jun 28, 2023

svanherk Jul 11, 2023

svanherk Jun 28, 2023

svanherk Jul 11, 2023

svanherk Jul 13, 2023

svanherk Jul 13, 2023

svanherk Jul 13, 2023

svanherk Jul 13, 2023

GZolla Jul 13, 2023

svanherk Jul 13, 2023

svanherk left a comment

svanherk Jul 14, 2023

svanherk commented Jul 14, 2023 •

edited

Loading

dbatiste commented Jul 14, 2023

ghost commented Jul 14, 2023

svanherk commented Jul 17, 2023

	pagerHasMore: { type: Boolean, attribute: 'pager-has-more', reflect: true },
	hasMore: { type: Boolean, attribute: 'has-more', reflect: true },

	{ key: 'how-t selected:false,o', text: 'How To Write a How To Article With a Flashy Title' },
	{ key: 'how-to', selected:false, text: 'How To Write a How To Article With a Flashy Title' },

	* Wether the dimension has more values to load
	* Whether the dimension has more values to load. Must be used with selected-first and search-type="manual"

feat: add load more pager to filters #3760

feat: add load more pager to filters #3760

Conversation

GZolla commented Jun 20, 2023

github-actions bot commented Jun 20, 2023

dbatiste Jun 21, 2023 • 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

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

github-actions bot commented Jun 27, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svanherk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svanherk commented Jul 14, 2023 • edited Loading

dbatiste commented Jul 14, 2023

ghost commented Jul 14, 2023

svanherk commented Jul 17, 2023

dbatiste Jun 21, 2023 •

edited

Loading

svanherk commented Jul 14, 2023 •

edited

Loading