Break apart FilterableListControls #4771

anselmbradford · 2019-01-17T00:13:57Z

FilterableListControls contains the controls, notifications, results and pagination. It really should instead have a parent module, FilterableList, that includes those elements.

Additions

Adds FilterableList module to encompass FilterableListControls, Notification, the results (post previews, activity log, or info units), and pagination.

Removals

filterable-list-controls.less and moves margin logic to block and the JS.
Removes filter_type option (see changes from Remove form_type from FilterableList organism #4774)

Changes

Removes filterable list template render macro and calls value values directly, like other templates that render directly from the block.
Update cucumber feature file to remove 0 from dates.
Hide result list when there are errors on the client-side.
Add label to server-side field error message, for consistency with client-side error message (see note section below though for differences that remain).

Testing

Pull branch and run gulp clean && gulp build
Check the following states.

No posts state

Example URL: http://localhost:8000/about-us/events/
No content shown.

		notification				results
	filter control	single field error	multi-field error	⚠	✓	post-preview	activity-log	info-unit	pager
NO-JS
JS

Example URL: http://localhost:8000/about-us/events/
Content for "No posts message" in Wagtail added.

		notification				results
	filter control	single field error	multi-field error	⚠	✓	post-preview	activity-log	info-unit	pager
NO-JS				x†
JS				x†

† Notification shown is in the default gray state.

Page loaded state, no filter interaction.

Example URL: http://localhost:8000/about-us/newsroom/

		notification				results
	filter control	single field error	multi-field error	⚠	✓	post-preview	activity-log	info-unit	pager
NO-JS	x†					x			x
JS	x					x			x

† Is in expanded state.

Page loaded state, single filter applied.

Example URL: http://localhost:8000/about-us/newsroom/?categories=blog

		notification				results
	filter control	single field error	multi-field error	⚠	✓	post-preview	activity-log	info-unit	pager
NO-JS	x†				x	x			x
JS	x†				x	x			x

† Is in expanded state.

Page loaded state, filters applied, small result set.

Example URL: http://localhost:8000/about-us/newsroom/?categories=blog&topics=financial-education&authors=holly-petraeus

		notification				results
	filter control	single field error	multi-field error	⚠	✓	post-preview	activity-log	info-unit	pager
NO-JS	x†				x	x
JS	x†				x	x

† Is in expanded state.

Page loaded state, filters applied, no result set.

Example URL: http://localhost:8000/about-us/newsroom/?categories=blog&topics=financial-education&authors=holly-petraeus&from_date=2018

		notification				results
	filter control	single field error	multi-field error	⚠	✓	post-preview	activity-log	info-unit	pager
NO-JS	x†			x
JS	x†			x

† Is in expanded state.

Page loaded state, filters applied, erroneous entry (one field).

Example URL: http://localhost:8000/about-us/newsroom/?to_date=asdf

		notification				results
	filter control	single field error	multi-field error	⚠	✓	post-preview	activity-log	info-unit	pager
NO-JS	x†	x
JS	x†	x

† Is in expanded state.

Page loaded state, filters applied, erroneous entry (multiple fields).

Example URL: http://localhost:8000/about-us/newsroom/?to_date=asdf&from_date=asdf

		notification				results
	filter control	single field error	multi-field error	⚠	✓	post-preview	activity-log	info-unit	pager
NO-JS	x†		x
JS	x†		x

† Is in expanded state.

Page loaded state, activity-log.

Example URL: http://localhost:8000/activity-log/

		notification				results
	filter control	single field error	multi-field error	⚠	✓	post-preview	activity-log	info-unit	pager
NO-JS	x†						x		x
JS	x						x		x

† Is in expanded state.

Page loaded state, info-unit.

Example URL: http://localhost:8000/consumer-tools/everyone-has-a-story/

		notification				results
	filter control	single field error	multi-field error	⚠	✓	post-preview	activity-log	info-unit	pager
NO-JS	x†							x	x
JS	x							x	x

† Is in expanded state.

Notes

An additional edge-case only-JS behavior is to expand the filter and enter an erroneous value for one of the date filters. It should display an error message and hide the results. It's identical to "Page loaded state, filters applied, erroneous entry (one field)," but performed via JS instead of server-side. However, it differs in the wording of the error message:

Server-side:

Client-side:

This is due to how the backend and frontend store the label that is added to the error message. This should probably be adjusted to be equal in the future.

schaferjh · 2019-01-17T14:17:26Z

@anselmbradford 👍 all good.

anselmbradford · 2019-01-17T14:27:44Z

@schaferjh I was wondering about the first "No posts state" at the top. Do we want to have a state where the component doesn't render anything at all? What should be the behavior if a filter has no posts to filter and there has been no message set by content editors?

schbetsy · 2019-01-17T15:28:36Z

cfgov/jinja2/v1/_includes/organisms/filterable-list.html

+
+{# SHOW A NOTIFICATION IF THERE'S NOTHING TO FILTER. #}
+
+{% if value.no_posts_message %}


This if/elif conditional needs to be in the opposite order. Currently, if I fill in the no_posts_message field on the "Blog" page, for example (even if there are posts), it acts like there are no posts:

blergh, good catch. I pushed up a has_results variable and check that.

Updated to has_unfiltered_results to better describe it since it had a value when the filters were applied and the warning notification was showing. b322bf0

cfgov/jinja2/v1/_includes/organisms/filterable-list.html

chosak · 2019-01-17T15:41:31Z

What should be the behavior if a filter has no posts to filter and there has been no message set by content editors?

Should those fields (at least no_posts_message, and maybe no_post_explanation) be required? I know those were just added in #4758. @schbetsy was it a deliberate design decision to make those optional, or a decision of expediency, as adding them as required would have necessitated adding messages to all existing fields?

cfgov/jinja2/v1/_includes/organisms/filterable-list.html

anselmbradford · 2019-01-17T15:42:57Z

cfgov/jinja2/v1/_includes/organisms/filterable-list.html

+                {{ activity_list.render(posts) }}
+            {% elif value.output_5050 %}
+                {% from 'molecules/info-unit-2.html' import info_unit with context %}
+                <div class="o-info-unit-group u-mb30" data-qa-hook="image-text-50-50">


TODO: This whole block should go into its own template, but to manage the complexity of this PR best break things apart here and do a separate PR to separate this part out.

schaferjh · 2019-01-17T15:45:54Z

@anselmbradford would you post a screenshot?

schbetsy · 2019-01-17T15:52:15Z

@schbetsy was it a deliberate design decision to make those optional

@chosak I didn't see any value in making the no_posts_message and _explanation fields required. The only pages where the "No posts message" could reasonably show up are the events and open notices pages. Making the field required would be pointless for all the other pages, causing confusion and extra work. Having a possible (unlikely) state where nothing shows up doesn't seem like a problem to me.

chosak · 2019-01-17T15:52:57Z

@schbetsy thanks for the explanation, that makes sense.

anselmbradford · 2019-01-17T16:52:38Z

would you post a screenshot?

@schaferjh

There are events:

There are no events and no message setting in Wagtail:

There are no events and a message is set in Wagtail:

See Betsy's comment above too as it will reasonably only appear on events and open notices.

schbetsy · 2019-01-17T17:21:08Z

In my opinion, it would make more sense for the filter controls to be open when there is a validation error. The user will have to interact with the expandable to fix the error; why make them press the "+" button to open it?

anselmbradford · 2019-01-17T17:28:19Z

In my opinion, it would make more sense for the filter controls to be open when there is a validation error. The user will have to interact with the expandable to fix the error; why make them press the "+" button to open it?

Argh, yeah this is a mistake. Any behavior that isn't like production is probably a mistake. If I recall, the only difference is hiding the results when there're client-side errors to align with how it's done server-side.

schaferjh · 2019-01-17T17:29:27Z

@anselmbradford I agree with @schbetsy's thinking that no posts is not a common enough situation to be a required field for all filterable lists. In fact it's really only likely on the events page and rarely possible on open notices. So, to your specific question,

What should be the behavior if a filter has no posts to filter and there has been no message set by content editors?
It's okay for the page to render with the filter components and blankness in this case. It will be a spur to the content editor to remember to set a message in the optional field. Doesn't need to be programmatically decided given it's rarity.

Scotchester

This is a good step forward in making Filterable Lists more manageable 👍

None of my questions and comments are truly blocking, but I'm "requesting changes" until we get the open-when-filters-are-active issue fixed.

cfgov/v1/atomic_elements/organisms.py

Scotchester · 2019-01-17T15:23:18Z

cfgov/v1/atomic_elements/organisms.py

-#       atomic name used on the frontend of FilterableListControls,
-#       or vice versa.
-class FilterControls(BaseExpandable):
+class FilterableList(BaseExpandable):


This doesn't really need to subclass BaseExpandable, does it? We're probably never going to create a filterable list that uses different label, border, etc. options than our standard. May need to add expandable.js to the js array in this class, though, if we stop subclassing BaseExpandable?

Filters should be untangled from expandables in multiple ways. I think that's out of scope here, but duly noted!

cfgov/jinja2/v1/_includes/organisms/filterable-list.html

Scotchester · 2019-01-17T17:30:09Z

cfgov/jinja2/v1/_includes/organisms/filterable-list.html

+
+            {% set total_pages = posts.paginator.num_pages %}
+            {% set current_page = posts.number %}
+            {% if total_pages > 1 and current_page <= total_pages %}


This condition seems superfluous?

Suggested change

{% if total_pages > 1 and current_page <= total_pages %}

{% if total_pages > 1 %}

Hmmmmm okay I've removed it in 9086a5f

This is actually a direct duplicate from https://github.com/cfpb/cfgov-refresh/blob/master/cfgov/jinja2/v1/_includes/molecules/pagination.html#L26 that is necessary to hide the pagination container, which is kinda a bummer.

It seems unnecessary for that to exist there. Seems like it should be up to the thing calling pagination to know if it actually should be calling it or not.

Possibly, the method signature looks pretty clear there and it's nice it handles the arguments coming into it correctly.

anselmbradford · 2019-01-17T18:37:49Z

In my opinion, it would make more sense for the filter controls to be open when there is a validation error. The user will have to interact with the expandable to fix the error; why make them press the "+" button to open it?

@schbetsy This is fixed in f0a1825, I thought has_active_filters was coming through value, but it's on it's own. Should it be global like that or should it be attached to the value on the backend?

EDIT: cc @Scotchester

anselmbradford · 2019-01-17T19:14:15Z

It's okay for the page to render with the filter components and blankness in this case. It will be a spur to the content editor to remember to set a message in the optional field. Doesn't need to be programmatically decided given it's rarity.

@schaferjh So the other option is to have a default message, but the issue there is what the text would say that would cover the possible cases for the no results.

The problem with completely blank from a frontend perspective is that we can't always assume that a filterable list will occupy space on the page, so have to be aware that there's an edge-case in regard to padding in the layout. Currently with it empty an empty div is added to the page markup that occupies space.

Scotchester · 2019-01-17T20:03:27Z

This is fixed in f0a1825, I thought has_active_filters was coming through value, but it's on it's own. Should it be global like that or should it be attached to the value on the backend?

I like it being separate and leaving value reserved for block field values.

schaferjh · 2019-01-17T21:54:46Z

@anselmbradford hmmm, if that's a problem then @ascott1 might be the better person to weigh in. From a UX perspective, it's such an edge case, I do not see a need to solve for it.

chosak · 2019-01-18T15:02:57Z

It occurs to me that renaming a block type like this (FilterControls to FilterableList -- a nice improvement) does require manually running the block_inventory Django management command (provided by wagtail-inventory) to regenerate the page/block list. This powers the "Block Inventory" options in Wagtail admin Settings.

Unfortunately this isn't currently documented in wagtail-inventory, so I've opened cfpb/wagtail-inventory#19 there.

cfgov/jinja2/v1/_includes/organisms/filterable-list.html

Choosing PDF type is obsolete and no longer supported.

Remove form_type from FilterableList organism

schbetsy

I haven't reviewed the JS, but this is looking good in the browser.

Scotchester

I didn't go through the JS with a fine-toothed comb, either, but everything looks good on a skim, and all the example pages work well 👍

anselmbradford · 2019-01-18T20:56:54Z

I defer all JS errors this has introduced to @schbetsy and @Scotchester 😆 Have a good long weekend!

anselmbradford requested review from chosak, Scotchester, mistergone, niqjohnson and schbetsy January 17, 2019 00:13

anselmbradford force-pushed the filterable_list_results_container branch 2 times, most recently from fc64493 to 9895f06 Compare January 17, 2019 00:28

Break apart FilterableListControls

05af0c0

anselmbradford force-pushed the filterable_list_results_container branch from 9895f06 to 05af0c0 Compare January 17, 2019 00:29

schbetsy reviewed Jan 17, 2019

View reviewed changes

anselmbradford commented Jan 17, 2019

View reviewed changes

cfgov/jinja2/v1/_includes/organisms/filterable-list.html Outdated Show resolved Hide resolved

anselmbradford commented Jan 17, 2019

View reviewed changes

anselmbradford added 2 commits January 17, 2019 12:04

Add has_results variable

05db670

Add code comments for empty if statements

347a2d4

Scotchester suggested changes Jan 17, 2019

View reviewed changes

Fix issue where has_active_filters were not being looked up correctly

f0a1825

anselmbradford added 2 commits January 17, 2019 14:19

Update filterable list meta label to correct name

90d0b69

Updated code comments

3f3c4bb

anselmbradford added 4 commits January 17, 2019 14:28

Space and strip spacing in if statements

f716331

Remove unused variable

da67b78

Enclose results in if statement | rename has_results variable

b322bf0

Remove superfluous condition from pagination rendering

9086a5f

anselmbradford and others added 2 commits January 17, 2019 17:31

Fixes missing value

f41f811

Merge branch 'master' into filterable_list_results_container

27a2afc

chosak mentioned this pull request Jan 18, 2019

Remove form_type from FilterableList organism #4774

Merged

11 tasks

schbetsy reviewed Jan 18, 2019

View reviewed changes

cfgov/jinja2/v1/_includes/organisms/filterable-list.html Outdated Show resolved Hide resolved

anselmbradford and others added 6 commits January 18, 2019 10:34

Update code comment

dee1b31

remove choice of form

ba1232a

Choosing PDF type is obsolete and no longer supported.

add migration

2249557

posts is always defined

f02b1e2

Merge pull request #4774 from chosak/only-one-form-type

86455b6

Remove form_type from FilterableList organism

Merge branch 'master' into filterable_list_results_container

27b17b2

schbetsy approved these changes Jan 18, 2019

View reviewed changes

Scotchester approved these changes Jan 18, 2019

View reviewed changes

Merge branch 'master' into filterable_list_results_container

9042909

anselmbradford merged commit 1ed6970 into master Jan 18, 2019

anselmbradford deleted the filterable_list_results_container branch January 18, 2019 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break apart FilterableListControls #4771

Break apart FilterableListControls #4771

anselmbradford commented Jan 17, 2019 •

edited by chosak

Loading

schaferjh commented Jan 17, 2019

anselmbradford commented Jan 17, 2019

schbetsy Jan 17, 2019

anselmbradford Jan 17, 2019

anselmbradford Jan 17, 2019

chosak commented Jan 17, 2019

anselmbradford Jan 17, 2019

schaferjh commented Jan 17, 2019

schbetsy commented Jan 17, 2019 •

edited

Loading

chosak commented Jan 17, 2019

anselmbradford commented Jan 17, 2019

schbetsy commented Jan 17, 2019

anselmbradford commented Jan 17, 2019

schaferjh commented Jan 17, 2019

Scotchester left a comment

Scotchester Jan 17, 2019

anselmbradford Jan 17, 2019

Scotchester Jan 17, 2019

anselmbradford Jan 17, 2019

Scotchester Jan 17, 2019

anselmbradford Jan 17, 2019

anselmbradford commented Jan 17, 2019 •

edited

Loading

anselmbradford commented Jan 17, 2019

Scotchester commented Jan 17, 2019

schaferjh commented Jan 17, 2019

chosak commented Jan 18, 2019

schbetsy left a comment

Scotchester left a comment

anselmbradford commented Jan 18, 2019


		{# SHOW A NOTIFICATION IF THERE'S NOTHING TO FILTER. #}

		{% if value.no_posts_message %}

	{% if total_pages > 1 and current_page <= total_pages %}
	{% if total_pages > 1 %}

Break apart FilterableListControls #4771

Break apart FilterableListControls #4771

Conversation

anselmbradford commented Jan 17, 2019 • edited by chosak Loading

Additions

Removals

Changes

Testing

No posts state

Page loaded state, no filter interaction.

Page loaded state, single filter applied.

Page loaded state, filters applied, small result set.

Page loaded state, filters applied, no result set.

Page loaded state, filters applied, erroneous entry (one field).

Page loaded state, filters applied, erroneous entry (multiple fields).

Page loaded state, activity-log.

Page loaded state, info-unit.

Notes

schaferjh commented Jan 17, 2019

anselmbradford commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chosak commented Jan 17, 2019

Choose a reason for hiding this comment

schaferjh commented Jan 17, 2019

schbetsy commented Jan 17, 2019 • edited Loading

chosak commented Jan 17, 2019

anselmbradford commented Jan 17, 2019

schbetsy commented Jan 17, 2019

anselmbradford commented Jan 17, 2019

schaferjh commented Jan 17, 2019

Scotchester 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anselmbradford commented Jan 17, 2019 • edited Loading

anselmbradford commented Jan 17, 2019

Scotchester commented Jan 17, 2019

schaferjh commented Jan 17, 2019

chosak commented Jan 18, 2019

schbetsy left a comment

Choose a reason for hiding this comment

Scotchester left a comment

Choose a reason for hiding this comment

anselmbradford commented Jan 18, 2019

anselmbradford commented Jan 17, 2019 •

edited by chosak

Loading

schbetsy commented Jan 17, 2019 •

edited

Loading

anselmbradford commented Jan 17, 2019 •

edited

Loading