New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve presentation of temporary filter #1390

Merged
merged 22 commits into from Jan 9, 2019

Conversation

Projects
None yet
4 participants
@cproensa
Copy link
Contributor

cproensa commented Sep 13, 2018

Relevant changes:

commit 93bda1b
Improve presentation of temporary filters

- Show a visual indication in the filter widget icon when the active
filter is a temporary filter
- Create a filter action to make the current temporary filter a
  persistent filter

Fixes: #0024775

commit 0fc7bb7
Fix switching view mode for temporary filters

Fix switching view mode for a temporary filter.
Previously, switching the advanced or simple view for a temporary
filter, would lose the filter.

Fixes: #0024776

commit 151719b
Add actions as buttons in filter dialog

Add some actions as buttons placed in the filter dialog working area.
Replace old buttons having verbose decription by new iconized buttons,
for actions "reset filter" and "save filter".
Include new action button for persisting a temporary filter.

commit 95a60b0
Clean up filter dialog code

Remove unused function parameters

$p_page_number is not needed, the filter does not manage pagination
$p_for_screen is not used
$p_expanded is not used. If a reduced form variant is ever needed, it
will be a better approach to write new code that manages the collapsed
state.

Remove unused form inputs

view_all_set.php?f=3 the "f" parameter is not used
"print" form parameter is not used
"offset" form parameter is not used
"page_number" form parameter is no used

commit 6e3d83c
Create view_all_set.php constants

Replace magic numbers with constants in view_all_set.php for filter
actions.

commit 8736034
Rearrange filter dialog widget

There were some issues withthe filter dialog:
- The form tags are not placed correctly in the layout, crossing inner
  div boundaries.
- The section at the bottom is not hidden when the widget is collapsed.
- Only one form is used, but there are several actions that don't need
  to share all form fields.

In order to improve some of the dialog options, the layour has been
rearranged:

- Move the buttons toolbar to the top of the widget, following the
  general style of other widgets.
- Move the "text search" field inside the filter parameters area. This
  is part of the filter properties and should be managed and rendered by
the dedicated code.
- Assign each area its own form. Time ago, the filter submit would parse
  all the fields as a whole, but now there's the option to evaluate only
the submitted fields, incrementally over the affected filter. This
allows to separate the header search field, filter load dorwpdowns, etc,
into individual forms.

Fixes: #25109
@cproensa

This comment has been minimized.

Copy link
Contributor

cproensa commented Sep 13, 2018

Minimal changes to the filter dialog:

  • a modifier for the filter icon when the filter is temporary
  • a new option to make the filter persistent

menu_308

The option to make the filter persistent could be made to be faster access. For example, placing a pushpin (or other icon) button in the widget bar?

@cproensa cproensa changed the title 0024775 improve presentation temporary filter Improve presentation of temporary filter Sep 14, 2018

@atrol

This comment has been minimized.

Copy link
Member

atrol commented Sep 18, 2018

Tested a bit and didn't encounter any error so far.

I am not sure if users without some background knowledge of the reported issue will understand the UI.

They might notice the small clock icon, they might get the tool tip (not on touch only devices) but will they understand what it means and will they find the new menu entry to influence it?

TBH I have no other/better idea at the moment.

Show resolved Hide resolved core/filter_api.php Outdated
Show resolved Hide resolved core/filter_api.php Outdated
@@ -1259,6 +1259,8 @@ $s_manage_filter_page_title = 'Manage Filters';
$s_manage_filter_edit_page_title = 'Edit filter';
$s_delete_filter_button = 'Delete';
$s_apply_filter_button = 'Apply';
$s_temporary_filter = 'Temporary filter';
$s_set_as_persistent_filter = 'Set as persistent filter';

This comment has been minimized.

@atrol

atrol Sep 18, 2018

Member

Maybe a native speaker can comment.
Shouldn't it be Set as persisted Filter?

This comment has been minimized.

@dregad

dregad Sep 21, 2018

Member

I think persistent is correct in this context. I would say Save as persistent filter though.

This comment has been minimized.

@vboctor

vboctor Dec 23, 2018

Member

I believe we have Temp - Persisted - Saved/Named. I generally worry about the distinction being too subtle for the users and for the translators that translate these terms into other languages without a lot of context.

Show resolved Hide resolved view_all_set.php Outdated
@dregad

This comment has been minimized.

Copy link
Member

dregad commented Sep 21, 2018

The option to make the filter persistent could be made to be faster access. For example, placing a pushpin (or other icon) button in the widget bar?

@cproensa Following up on @atrol's remarks on UX, I agree that even though it works fine, the UI can be somewhat confusing for users who are not aware of the background.

What about making the "filter with clock" icon active as a button to save the filter (with pop-up text like This is a temporary filter, click to save it or something along those lines ? This could be a replacement or in addition to the menu option under the hamburger icon.

@cproensa cproensa force-pushed the cproensa:0024775_improve_presentation_temporary_filter branch from def09fd to 66cfb1c Oct 18, 2018

@cproensa

This comment has been minimized.

Copy link
Contributor

cproensa commented Oct 18, 2018

I am not sure if users without some background knowledge of the reported issue will understand the UI.

I didn't intend to make any UI changes that may break current styling. That is a sensitive topic and i am not confident to push that one through by myself.
The relevant change is to enable the action, which now can be repeated with the same request as the option i added to the drop-down menu, in any other UI control that is proposed.

What about making the "filter with clock" icon active as a button to save the filter (with pop-up text like This is a temporary filter, click to save it or something along those lines ? This could be a replacement or in addition to the menu option under the hamburger icon.

The clock icon is well suited as "informative" of filter's current nature. The action to persist it is not well represented by the clock. I thought of the thumb-tack icon, but placing that one on the widget bar is more commonly related to fixing or anchoring a widget/window, so it may be confusing.

@dregad

This comment has been minimized.

Copy link
Member

dregad commented Oct 19, 2018

The action to persist it is not well represented by the clock.

Hence my suggestion to have an explanatory pop-up, both to enhance the meaning of the "filter with clock" icon, and to explain how to make the temp filter permanent.

I agree that adding a thumb-tack icon on the toolbar may be even more confusing than the way you've implement it here.

@cproensa

This comment has been minimized.

Copy link
Contributor

cproensa commented Oct 29, 2018

there has been some discussion on https://mantisbt.org/bugs/view.php?id=24549
about presentation.
Can you have a look to see if it's viable?

@dregad

This comment has been minimized.

Copy link
Member

dregad commented Nov 30, 2018

As mentioned in the tracker, I'm OK with this change but would prefer to have the enhanced UI included as part of this PR.

@cproensa cproensa force-pushed the cproensa:0024775_improve_presentation_temporary_filter branch from 66cfb1c to a9cbbc8 Dec 23, 2018

@cproensa

This comment has been minimized.

Copy link
Contributor

cproensa commented Dec 23, 2018

Updated PR with a deeper changes in the filter dialog UI.
A considerable clean up was needed, in order to correctly implement the new changes. The layout for the widget was outdated and had some errors and inconsistencies

Standard filter:
2018-12-23_02 23 19

Temporary filter:
2018-12-23_02 23 34

Collapsed widget:
2018-12-23_02 17 23

@cproensa cproensa closed this Dec 23, 2018

@cproensa cproensa reopened this Dec 23, 2018

@atrol

This comment has been minimized.

Copy link
Member

atrol commented Dec 23, 2018

What about having the same UI for collapsed and non-collapsed view?
e.g. something like that
filtercollapsed

@cproensa

This comment has been minimized.

Copy link
Contributor

cproensa commented Dec 23, 2018

What about having the same UI for collapsed and non-collapsed view?

The framework hides the widget body when collapsed. The fact that we had something still there was due to the broken structure of the form tags, and making the browser to render the bottom div outside of the proper body.
So, in order to achieve your suggestion of collapsing but still showing something as "body", i fear we would need to fight against the framework.

I understand the point against crowding the widget bar with a lot of items. Note also that those can be selectively hidden at different viewport sizes.

@atrol

This comment has been minimized.

Copy link
Member

atrol commented Dec 23, 2018

I understand the point against crowding the widget bar

It's not that much against crowding the widget bar, it's more about

  • different UI's for the same functionality confuses user
  • the additional line with the Search label looks not that good and wastes some vertical space

Just brainstorming, maybe it's an option that the Search / Apply Filter / Reset / Load, ... line is no longer part of the collapsable area, but a standalone line or part of the Viewing Issues section below.

@vboctor
Copy link
Member

vboctor left a comment

Thanks @cproensa for the cleanup and improvements. I added some minor comments. Will try to look test the new UX and give feedback.

if( 0 == strcasecmp( 'off', $p_string ) || 0 == strcasecmp( 'no', $p_string ) || 0 == strcasecmp( 'false', $p_string ) || 0 == strcasecmp( '', $p_string ) || 0 == strcasecmp( '0', $p_string ) ) {
if( 0 == strcasecmp( 'off', $p_string )
|| 0 == strcasecmp( 'no', $p_string )
|| 0 == strcasecmp( 'n', $p_string )

This comment has been minimized.

@vboctor

vboctor Dec 23, 2018

Member

Do we need to add n as a separated value for false?

This comment has been minimized.

@vboctor

vboctor Dec 23, 2018

Member

Maybe we should just return directly the value of the expression rather than if (expr) return true else return false.

This comment has been minimized.

@cproensa

cproensa Dec 24, 2018

Contributor

iirc i added this as we already were using "temporary=y", which would resolve to true by default.

@@ -1259,6 +1259,8 @@ $s_manage_filter_page_title = 'Manage Filters';
$s_manage_filter_edit_page_title = 'Edit filter';
$s_delete_filter_button = 'Delete';
$s_apply_filter_button = 'Apply';
$s_temporary_filter = 'Temporary filter';
$s_set_as_persistent_filter = 'Set as persistent filter';

This comment has been minimized.

@vboctor

vboctor Dec 23, 2018

Member

I believe we have Temp - Persisted - Saved/Named. I generally worry about the distinction being too subtle for the users and for the translators that translate these terms into other languages without a lot of context.

Show resolved Hide resolved view_all_set.php Outdated
Show resolved Hide resolved view_all_set.php Outdated
Show resolved Hide resolved core/constant_inc.php Outdated
@cproensa

This comment has been minimized.

Copy link
Contributor

cproensa commented Dec 24, 2018

the additional line with the Search label looks not that good and wastes some vertical space

yes... on one side, my reasoning is that the "text search" is part of the filter properties, so its logical place is with the rest of the filter fields. One objective of the filter form is to be reusable, however at the moment, we have to render the search field separately from the main filter fields (eg, see also editing a stored filter). Being part of the filter fields is one step towards modularity of the form.
We could style it in some other way to stand out differently from the other fields. The issue about vertical space: we could also rearrange some other fields around it.

Just brainstorming, maybe it's an option that the Search / Apply Filter / Reset / Load, ... line is no longer part of the collapsable area, but a standalone line or part of the Viewing Issues section below.

i think that is even worse, as we would be crossing functions over different containers.

@cproensa

This comment has been minimized.

Copy link
Contributor

cproensa commented Dec 24, 2018

Updated to keep the search field as previously placed. For view issues filtering

imagen

@atrol

This comment has been minimized.

Copy link
Member

atrol commented Dec 27, 2018

  • when being logged in as anonymous user, persisting filters is offered (remove it?), but does not work
  • list of filters contains not needed entry [Reset Filter] in collapsed mode

@cproensa cproensa force-pushed the cproensa:0024775_improve_presentation_temporary_filter branch from fe7e93c to 998b24f Dec 27, 2018

@dregad
Copy link
Member

dregad left a comment

I tested this today and quite like what you did @cproensa - thanks !

One thing that bothers me a little is the widgets' order on the title bar in collapsed mode. I think that for consistency they should appear in the same overall sequence as when the filters section is expanded (Persist / Reset / Save / Load / Search / Apply).

selection_007

I'm not sure the separator between Load Filter and Search is necessary.

Also, what is the reason for not having a Save button in collapsed mode ?

I'll push a few commits with some tweaks shortly.

@@ -72,16 +72,16 @@
if( $f_isset_temporary ) {
# when only changing the temporary status of a filter and no action is specified
# we assume not to reset current filter
if( $f_type < 0 ) {
if( $f_type == -1 ) {

This comment has been minimized.

@dregad

dregad Dec 30, 2018

Member

magic constant -1 should be defined too.

This comment has been minimized.

@cproensa

cproensa Dec 30, 2018

Contributor

-1 is not an actual value, it's a forced return by gpc_get_xxx to detect that the parameter has not been used.
We could use a different literal, but i don't think it should be it's own filter related constant.

Show resolved Hide resolved core/constant_inc.php Outdated
}
$t_temp_filter = $f_make_temporary;
} else {
$t_temp_filter = filter_is_temporary( $t_setting_arr );
}
if( $f_type < 0 ) {
if( $f_type == -1 ) {

This comment has been minimized.

@dregad

dregad Dec 30, 2018

Member

ditto

Show resolved Hide resolved core/gpc_api.php Outdated
@cproensa

This comment has been minimized.

Copy link
Contributor

cproensa commented Dec 30, 2018

I think that for consistency they should appear in the same overall sequence as when the filters section is expanded (Persist / Reset / Save / Load / Search / Apply).

Yes, that seems a good suggestion.

I'm not sure the separator between Load Filter and Search is necessary.

The order, and the separator, were already there, so did'nt think much about further changes. Probably we could get rid of that separator too.

Also, what is the reason for not having a Save button in collapsed mode ?

My thinking was to not add too much stuff in the widget bar. Saving a filter is usually an action done after tweaking the filter parameters, so in that scenario, the widget should be expanded to do that.
And in any case, is an action that should not be that frequently used to have its space in the collapsed bar.

@atrol

atrol approved these changes Jan 7, 2019

@cproensa

This comment has been minimized.

Copy link
Contributor

cproensa commented Jan 7, 2019

the discussion is no reason to prevent merging.

Well, preferably, we should make any change to lang strings and its var names before merging

@dregad

This comment has been minimized.

Copy link
Member

dregad commented Jan 7, 2019

In my opinion, there is no need to append "filter" on each button/label - it is redundant and quite clear from context what we are working with (it says so in the title bar right above the buttons / select). Unless these strings are also used elsewhere with a less obvious intention.

I agree with @cproensa's suggestion that the select's label can be shortened to Load; in that case the language string should probably be changed from $s_load_filter to $s_load (and maybe the same for $s_persist_filter too, for consistency).

Note that the "Reset Filter" language string (reset_query) is used as tooltip (see b54c1ca) so this one should probably be kept.

@cproensa cproensa force-pushed the cproensa:0024775_improve_presentation_temporary_filter branch from 6ae02b9 to 21ca1e1 Jan 7, 2019

@cproensa

This comment has been minimized.

Copy link
Contributor

cproensa commented Jan 7, 2019

I agree with @cproensa's suggestion that the select's label can be shortened to Load;

it was atrol's suggestion...

Thanks for the feedback. I've deleted the previous commit and modified to the short "load" string.
So now it stays with the short, one word, action buttons/labels.

cproensa and others added some commits Sep 13, 2018

Allow y/n strings for gpc_string_to_bool
Add options for reading "y" and "n" as boolean parameters in
function gpc_string_to_bool
Improve presentation of temporary filters
- Show a visual indication in the filter widget icon when the active
filter is a temporary filter
- Create a filter action to make the current temporary filter a
  persistent filter

Fixes: #0024775
Set explicit project id for filter update
Set the explicit project id for updating the filter, to avoid situations
where the current project has been modified concurrently after loading
the form page.
Fix switching view mode for temporary filters
Fix switching view mode for a temporary filter.
Previously, switching the advanced or simple view for a temporary
filter, would lose the filter.

Fixes: #0024776
Add actions as buttons in filter dialog
Add some actions as buttons placed in the filter dialog working area.
Replace old buttons having verbose decription by new iconized buttons,
for actions "reset filter" and "save filter".
Include new action button for persisting a temporary filter.
Remove redundant reset filter actions
Remove the "reset filter" entry in the filter selection dropdown, as we
now have a dedicated action button.
The old button for "reset filter" was being showed when the filter
selection was hidden, but it's not needed now as the new action button
is always displayed.
Clean up filter dialog code
Remove unused function parameters

$p_page_number is not needed, the filter does not manage pagination
$p_for_screen is not used
$p_expanded is not used. If a reduced form variant is ever needed, it
will be a better approach to write new code that manages the collapsed
state.

Remove unused form inputs

view_all_set.php?f=3 the "f" parameter is not used
"print" form parameter is not used
"offset" form parameter is not used
"page_number" form parameter is no used
Create view_all_set.php constants
Replace magic numbers with constants in view_all_set.php for filter
actions.
Rearrange filter dialog widget
There were some issues withthe filter dialog:
- The form tags are not placed correctly in the layout, crossing inner
  div boundaries.
- The section at the bottom is not hidden when the widget is collapsed.
- Only one form is used, but there are several actions that don't need
  to share all form fields.

In order to improve some of the dialog options, the layour has been
rearranged:

- Move the buttons toolbar to the top of the widget, following the
  general style of other widgets.
- Move the "text search" field inside the filter parameters area. This
  is part of the filter properties and should be managed and rendered by
the dedicated code.
- Assign each area its own form. Time ago, the filter submit would parse
  all the fields as a whole, but now there's the option to evaluate only
the submitted fields, incrementally over the affected filter. This
allows to separate the header search field, filter load dorwpdowns, etc,
into individual forms.

Fixes: #25109
Apply uniform color to filter titles
Apply uniform blue color for filter properties titles whether they are a
link or not.
Revert search field to its original position
Due to concerns about the vertical space occupied by the filter box, now
when rendering the filter widget for view issues page, the text search
field is back to its original position below the fields table, in line
with the form submit button.
Remove persist filter option for anonymous user
Users that can't use persistent filters (currently, anonymous user),
should not be presented with the option to do so.
Filters contains unneeded reset entry in collapsed mode.
The [reset filter] option in dropdown list, when the widget is
collapsed, is no longer needed, as now there is a dedicated button.
Refactor gpc_string_to_bool
Avoid repeated calls to strcasecmp

@dregad dregad force-pushed the cproensa:0024775_improve_presentation_temporary_filter branch from f599f70 to 11f29fc Jan 9, 2019

@dregad dregad merged commit 11f29fc into mantisbt:master Jan 9, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

dregad added a commit that referenced this pull request Jan 9, 2019

Improve presentation of temporary filters
Merge PR #1390

Fixes #24775, #24776, #25109
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment