show/hide filter state inputs #181

asbates · 2023-01-31T21:06:55Z

This is the first part of #129, allowing filter states to show either the inputs or a summary, with only one input visible at a time. This is done through a redesign of the filter cards and their container. The cards now have a header that displays the summary information. When a header is clicked, the input expands. Clicking a different header closes the first and expands the second. The 2 step process is so that the functionality can be introduced so other issues are not held up.

For part 2, I am planning on improving the styling. More specifically, truncating the filter state value when it is long. e.g. categorical with 20 levels selected needs to be shortened somehow. I would also like to make some general style updates to make it look nicer.

gogonzo

Looks good. This is a good way to solve out problems. Let's discuss the way how new FS are going to be added/inserted.

I think that to complete this issue we will have to use renderUI instead of insertUI/removeUI #135
I'm happy to discuss this on the call

gogonzo · 2023-02-01T09:05:53Z

R/FilterStateChoices.R

+    ui = function(id) {
+      ns <- NS(id)
+
+      tags$li(
+        id = id,
+        tags$div(
+        class = "filter-card filter-card-range",
+        tags$div(
+          class = "filter-card-header",
+          uiOutput(ns("header_name_value"), inline = TRUE),
+          uiOutput(ns("header_keep_na"), inline = TRUE),
+          tags$div(
+            class = "filter-card-icons",
+            tags$span(
+              class = "filter-card-toggle fa fa-chevron-right"
+            ),
+            actionLink(
+              inputId = ns("remove"),
+              label = icon("circle-xmark", lib = "font-awesome"),
+              class = "filter-card-remove"
+            )
+          )
+        ),
+        tags$div(
+          class = "filter-card-body",
+          private$ui_inputs(ns("inputs"))
+        )
+        )
+      )


I think we can keep ui in the FilterState as before but header could be delegated to the child-classes - in the FIlterState it could look like this:

ui = div( class = "filter-card-header", private$ui_header(ns("header")), actionLink(ns("remove"),...) ) div( private$ui_inputs(ns("inputs")) )

I second the opinion about keeping the ui in the FilterState since the code seems reusable and all the FilterStateXXX inherits from FilterState.

I also see reusable code in server method. Maybe we can move that to FilterState class too?

Definitely some repeated code. I think I misunderstood a conversation the other day.

gogonzo · 2023-02-01T09:10:12Z

R/FilterStateRange.R

+          uiOutput(ns("header_name_value"), inline = TRUE),
+          uiOutput(ns("header_keep_na"), inline = TRUE),
+          uiOutput(ns("header_keep_inf"), inline = TRUE),


Maybe instead of:
<varname>: min-max NA: TRUE Inf: TRUE
<varname>: min-max NA: FALSE Inf: FALSE

following could be better:
<varname>: min-max, NA, Inf
<varname>: min-max

Not sure what you mean here. Could you expand on this?

Sorry, I was wrong above. I put <varname>: instead of selected:.

NA: TRUE and Inf: TRUE takes some space so I think about alternative display. Instead of having NA: TRUE as a separate "span" we can include it in the selected "span", so we could have:

selected: F, M, NA when NA is TRUE

selected: 18-64, Inf when Inf is TRUE

selected: 18-64 when Inf is FALSE

In above examples word selected could be replaced with choices

gogonzo · 2023-02-01T09:12:12Z

inst/css/filter-panel.css

+  color: var(--bs-danger, #a94442);
+}
+
+.filter-card-body {


We need some margin

nikolas-burkoff · 2023-02-01T11:03:00Z

On the refactor branch I had to add include_css_files(pattern = "filter-panel"), inside the UI of the FilterState class see here to make sure that the apps referenced here have appropriate css.

Sorry that has caused a conflict here.

We should make sure these apps work with the new js/css

nikolas-burkoff · 2023-02-01T11:55:52Z

Looking really nice! 👍

I've been playing with examples #165

Might be worth thinking of:

rounding of numbers
I like an arrow for a range so "10 -> 1000" rather than just (10 - 1000)
We can open multiple accordions at once:

I don't know if that's a problem or not @gogonzo
For the header which is too long - I quite like the idea of "VARNAME x, y,... (+23)" or something like that
When you first open a range filter I don't see the histogram until I start moving the slider
When it comes to styling we should decide which bootstrap versions we are going to support, for example I could imagine setting a default bootstrap when teal is loaded and that's the one we fully support - as it's going to get painful supporting multiple versions if there's not a need to @donyunardi

donyunardi · 2023-02-01T19:13:15Z

When it comes to styling we should decide which bootstrap versions we are going to support, for example I could imagine setting a default bootstrap when teal is loaded and that's the one we fully support - as it's going to get painful supporting multiple versions if there's not a need to @donyunardi

For now, I think we should focus on the the default bootstrap (for shiny 1.6.0, it's bootstrap 4) that is being used by shiny/teal. This way we won't be distracted as we continue on with our progress. Once we're done with the code foundation of the upgraded filter panel, we can revisit this and see how teal reacts with other bs version.

donyunardi · 2023-02-01T19:44:35Z

Thanks @asbates

The text for showing the current selection could be too long.

Maybe we split it to two rows, one for datetime value and the second line for NA and Inf?

Although, COUNTRIES can also make it too long if users select multiple countries.

Unless there's a way to make this more pleasant, I am leaning toward not include the selection values in the title.
Since this is user facing, tagging @lcd2yyz for opinion.

asbates · 2023-02-01T21:08:53Z

For now, I think we should focus on the the default bootstrap (for shiny 1.6.0, it's bootstrap 4) that is being used by shiny/teal. This way we won't be distracted as we continue on with our progress. Once we're done with the code foundation of the upgraded filter panel, we can revisit this and see how teal reacts with other bs version.

So FYI as of 1.6.0 Shiny supports bootstrap 4. But it still uses 3 as the default.

asbates · 2023-02-01T21:11:28Z

Unless there's a way to make this more pleasant, I am leaning toward not include the selection values in the title. Since this is user facing, tagging @lcd2yyz for opinion.

Having only the variable name would definitely solve this and would be the easiest route. I was planning on doing some abbreviation to resolve though.

…_panel_refactor@main Signed-off-by: Andrew Bates <andrew.bates@atorusresearch.com>

donyunardi · 2023-02-01T21:24:34Z

For now, I think we should focus on the the default bootstrap (for shiny 1.6.0, it's bootstrap 4) that is being used by shiny/teal. This way we won't be distracted as we continue on with our progress. Once we're done with the code foundation of the upgraded filter panel, we can revisit this and see how teal reacts with other bs version.

So FYI as of 1.6.0 Shiny supports bootstrap 4. But it still uses 3 as the default.

Ah okay, thanks. Yes, let's focus on 3 then.

lcd2yyz · 2023-02-01T23:02:55Z

Unless there's a way to make this more pleasant, I am leaning toward not include the selection values in the title. Since this is user facing, tagging @lcd2yyz for opinion.

I also feel including selected values in the variable names make it way to crowded. We don't need to go so extreme to collapse to one-liner, but being able to show the 1) variable label and 2) selected values or ranges, will be informative and important.

So this is what I had in mind initially, it should also allow bootstrap theme to come through

I'm less concerned about showing NA/Inf in the collapsed mode - if visually doesn't fit (in a visually pleasing way), we don't need to show. But I think we might need to design different displays for different variable class. For example

Boolean var: show any selected
Categorical var:
- Binary: show any selected (same as boolean)
- Multiple: show selected if only 1 category checked, show as "Multiple" if >1 category selected
Numerical var: show selected range, auto formatted to scientific notation if significant digit is >4
Date var: show selected date range, formatted to ymd only (do not show time part)

gogonzo · 2023-02-06T16:33:12Z

We gathered with the team and decided to move on with the issue (on the feature branch). So that:

summary card will contain header + one-line-summary
header will be always the same for all filter states- Title + label + remove button
summary will always be a one-liner which will be produced by the XyzFilterState class. So each XyzFilterState class will be responsible to produce the summary.

So far we concluded to have following summary:

RangeFIlterState: $min-$max, if $na then "NA" else NULL, if $inf then "Inf" else NULL
DateFilterState: $min-$max, if $na then "NA" else NULL
ChoicesFilterState: $n-levels selected
DatetimeFilterState: $min-$max, if $na then "NA" else NULL
LogicalFilterState: $value, if $na then "NA" else NULL

If we stick to the convention that summary is always a one liner produced by the classes than it will be very easy to change particular summary module for a single data type without changing the design for all of them.

donyunardi

Thanks @asbates , provided my feedback.

donyunardi · 2023-02-06T22:03:05Z

R/FilterState.R

-            )
+              class = "filter-card-title",
+              tags$span(self$get_varname(deparse = TRUE)),
+              if (length(self$get_varlabel())) {


The varname and varlabel show twice when it holds the same value:

In the previous state of the code, we have the if statement comparing $get_varname and $get_varlabel, which will work, but I don't think this is the root cause.

private$varlabel is initialized with condition to not hold the same value with varname:

teal.slice/R/FilterState.R

Lines 91 to 96 in aab1255

private$varlabel <- if (identical(varlabel, as.character(varname))) {

# to not display duplicated label

character(0)

} else {

varlabel

}

However, when I browser() this, I see the following:

varlabel is the result of get_varlabels() method from FilteredDataset class:

teal.slice/R/FilteredDatasetDefault.R

Lines 41 to 50 in aab1255

private$add_filter_states(

filter_states = init_filter_states(

data = self$get_dataset(),

input_dataname = as.name(dataname),

output_dataname = as.name(dataname),

varlabels = self$get_varlabels(),

keys = self$get_keys()

),

id = "filter"

)

I think we need to review this method:

teal.slice/R/FilteredDataset.R

Lines 292 to 301 in aab1255

get_varlabels = function(variables = NULL) {

checkmate::assert_character(variables, null.ok = TRUE, any.missing = FALSE)

labels <- formatters::var_labels(private$dataset, fill = FALSE)

if (is.null(labels)) {

return(NULL)

}

if (!is.null(variables)) labels <- labels[intersect(self$get_varnames(), variables)]

labels

},

Maybe something changes in formatters::var_labels?

Wow, thanks for the details! I'll look into this.

donyunardi · 2023-02-06T22:18:39Z

R/FilterStateRange.R

+      ns <- NS(id)
+      uiOutput(ns("summary"), class = "filter-card-summary")
+    },
+    server_summary = function(id) {


We should hide the summary when the filter card is clicked:

donyunardi · 2023-02-06T22:23:22Z

inst/css/filter-panel.css

+  border-radius: 0.25rem;
+}
+
+.filter-card-header:hover {


Suggested change

.filter-card-header:hover {

.filter-card:hover {

On hover, I can see the header changes.

What do you think about applying this to the filter-card instead?

Good call. I'll look into this. Your screenshots look odd to me though because the hover color is supposed to apply to the title and summary. The idea is to focus on a card when it is collapsed. May be better on the whole card.

I looked at coloring the whole card on hover. This made it hard to separate the header from the body on an expanded card. I've added hover color for the body on an expanded card. This visually separates the header and body better. Please let me know what you think about this.

lcd2yyz · 2023-02-06T22:27:47Z

RangeFIlterState: $min-$max, if $na then "NA" else NULL, if $inf then "Inf" else NULL

DateFilterState: $min-$max, if $na then "NA" else NULL

ChoicesFilterState: $n-levels selected

DatetimeFilterState: $min-$max, if $na then "NA" else NULL

LogicalFilterState: $value, if $na then "NA" else NULL

@gogonzo Few minor questions on the defined filter states

any particular reason for differentiate date vs datetime as two filter states?
Assuming RangeFilterState is to be used for numerical variables, I have some concern on the naming itself, because ultimately dates are also expected as a range with min-max. Should we just call it "numerical" or are there some additional considerations?

gogonzo · 2023-02-07T07:36:25Z

any particular reason for differentiate date vs datetime as two filter states?

DateFilterState and DatetimeFilterState classes are separated as their ui/srv differ. Both classes use different functions to do stuff like formatting or handling timezones. We can't abstract these two classes into one

Assuming RangeFilterState is to be used for numerical variables, I have some concern on the naming itself, because ultimately dates are also expected as a range with min-max. Should we just call it "numerical" or are there some additional considerations?

We can change the names of the classes but it won't change anything for the end user as these are not exported. If we go this way, then there is no reason for a ChoicesFilterState and we might have to split it into two classes which would be FactorFilterState and CharacterFilterState.

gogonzo · 2023-02-07T08:06:47Z

inst/js/filter-panel.js

+
+// if 2., we need to set the active card, which was clicked, to inactive
+//  this is handled in the second 'if' block
+toggleFilterCard = function (el, containerId) {


In my browser after clicking card inputs overlap with the header and also summary doesn't disappear (I'm not sure if it should).

I think each filter-card could have a boarder also to separate visually from the others,

For border, I've made them darker so the visual separation is more obvious.

For overlap, that's going to take some digging. More generally the cards don't look right on Chrome so I'm guessing there's something going on with browser support.

Firefox:

Chrome:

Yes, there is something going on with the browser, but we can't use this excuse towards end-users. I'm fine with having this look on the feature branch "just to move on" but we have to be sure that we are able to fix it quickly. @asbates how do you think, is it doable to fix the appearance for chrome relatively fast?

What would be quick to you? I'm thinking the end of this week if I had to say. What's the alternative?

It's looking good now!

Almost there - pickerInput space inside of the filter-card-body is trimmed to the size of the card

R/FilterStates.R

lcd2yyz · 2023-02-07T15:01:13Z

@gogonzo Thanks for the explanation, makes sense. Let's keep your proposed design then.

asbates · 2023-02-07T20:10:57Z

@nikolas-burkoff

We can open multiple accordions at once

I was under the impression the one-at-a-time only applies within an accordion not across accordions. @gogonzo what do you think?

When you first open a range filter I don't see the histogram until I start moving the slider

I suspect this may be due to different browsers. Hoping this goes away with addressing other issues but keeping track of it just in case.

asbates · 2023-02-07T20:49:53Z

@lcd2yyz Regarding "auto formatted to scientific notation if significant digit is >4".

I've added rounding via sprintf("%.4g", value). R describes the "g" flag as "Double precision value, in %e or %E format if the exponent is less than -4 or greater than or equal to the precision, and %f format otherwise. (The precision (default 6) specifies the number of significant digits here, whereas in %f, %e, it is the number of digits after the decimal point.)".

I think this is close to what you're going for but not sure if it's exact. What do you think?

gogonzo · 2023-02-07T21:25:43Z

I was under the impression the one-at-a-time only applies within an accordion not across accordions. @gogonzo what do you think?

Across accordions would be better, but still we can live with accordion within a dataset. But on the other side, if we are running custom js and css just to create accordions within each FilterState, why not to use something ready-to-use like shiny.semantic::accordion or the bootstrap accordion:
@asbates let's meet tomorrow so we can both discuss this.

gogonzo · 2023-02-07T21:27:46Z

I've added rounding via sprintf("%.4g", value)

I think for a decimal point we can use options so the app developer can change it if has a specific data which require better precision. We shouldn't fix it to one value

teal.slice/R/zzz.R

Line 3 in 8a09ad5

teal_default_options <- list(teal.threshold_slider_vs_checkboxgroup = 5)

asbates · 2023-02-07T22:05:58Z

I was under the impression the one-at-a-time only applies within an accordion not across accordions. @gogonzo what do you think?

Across accordions would be better, but still we can live with accordion within a dataset. But on the other side, if we are running custom js and css just to create accordions within each FilterState, why not to use something ready-to-use like shiny.semantic::accordion or the bootstrap accordion: @asbates let's meet tomorrow so we can both discuss this.

We can absolutely use bootstrap instead. To be honest I didn't know that existed in bootstrap. The only thing is we would have to account for versions 3, 4, 5. I don't mind using another package like shiny.semantic either. Personally I would try to avoid adding that as a dependency just for this but I'll go with it if that's what's decided. I agree a discussion on implementation would be good.

nikolas-burkoff · 2023-02-08T08:30:40Z

Across accordions would be better

So the issue with having the possibility of each accordion having something open concerns cdisc data where filtering on ADSL triggers changes to the counts in other datasets - which brings in more reactivity (and possibly something similar within summarized experiments). However, if there's at most one filter card open on each dataset that will limit the reactivity substantially so it's probably ok

nikolas-burkoff · 2023-02-08T08:40:00Z

I think for a decimal point we can use options so the app developer can change it if has a specific data which require better precision. We shouldn't fix it to one value

And we should remember when possible to add the option into this vignette.

lcd2yyz · 2023-02-08T18:39:53Z

I've added rounding via sprintf("%.4g", value)

I think for a decimal point we can use options so the app developer can change it if has a specific data which require better precision. We shouldn't fix it to one value

teal.slice/R/zzz.R

Line 3 in 8a09ad5

teal_default_options <- list(teal.threshold_slider_vs_checkboxgroup = 5)

I feel this example is slightly different - this is controlling a conscious switch from one variable class to another, that is not the inherent property of the variable.
The decimal rounding is somewhat "native" to the property of numerical variables itself. While we can definitely add it as optional argument to teal start-up setting and make it as flexible as possible, on the other hand, too many choices/options isn't necessary a good thing. We've just talked about the amount of complexity we already have in the product, then following the path of simplification, I'm okay with implementing a fixed value for now, then come back in the future if there are actual user request for such flexibility.

In conclusion, what @asbates has proposed looks good to me.

asbates · 2023-02-08T19:04:08Z

I've added rounding via sprintf("%.4g", value)

I think for a decimal point we can use options so the app developer can change it if has a specific data which require better precision. We shouldn't fix it to one value

teal.slice/R/zzz.R

Line 3 in 8a09ad5

teal_default_options <- list(teal.threshold_slider_vs_checkboxgroup = 5)

I feel this example is slightly different - this is controlling a conscious switch from one variable class to another, that is not the inherent property of the variable. The decimal rounding is somewhat "native" to the property of numerical variables itself. While we can definitely add it as optional argument to teal start-up setting and make it as flexible as possible, on the other hand, too many choices/options isn't necessary a good thing. We've just talked about the amount of complexity we already have in the product, then following the path of simplification, I'm okay with implementing a fixed value for now, then come back in the future if there are actual user request for such flexibility.

In conclusion, what @asbates has proposed looks good to me.

@lcd2yyz I think this is just an example of using an option, not the option that would apply to this case. I will continue with a fixed value. I opened #190 to keep track of using an option.

filter card accordion

52434c0

nikolas-burkoff added the core label Feb 1, 2023

gogonzo reviewed Feb 1, 2023

View reviewed changes

linter

6737e3d

Merge branch 'filter_panel_refactor@main' into 129_hide_inputs@filter…

1bfc16c

…_panel_refactor@main Signed-off-by: Andrew Bates <andrew.bates@atorusresearch.com>

asbates mentioned this pull request Feb 3, 2023

init filter card design #168

Closed

asbates added 2 commits February 3, 2023 15:37

filter card styling

5533132

EmptyFilterState input styling

8edc319

asbates added 5 commits February 6, 2023 09:59

move card ui/server to FilterState abstract

56c68a8

refactor variable summary into filter state sub classes

7bd4a46

js adjustments

039bcf0

css adjustments

6602e6a

fix mae data label

4e181a0

donyunardi requested changes Feb 6, 2023

View reviewed changes

gogonzo reviewed Feb 7, 2023

View reviewed changes

R/FilterStates.R Outdated Show resolved Hide resolved

asbates added 2 commits February 7, 2023 09:40

use '_' in card container id instead of '-'

7bf467d

hide card summary on expand

4e342b2

asbates added 2 commits February 7, 2023 11:56

make card border darker for more obvious separation

bf453a7

color on card body hover for focus

93d83c5

round summary for numeric

dfa051d

fix header size issue on chrome

56bd507

asbates mentioned this pull request Feb 8, 2023

Add option for rounding in filter card summary #190

Open

asbates closed this Feb 13, 2023

asbates deleted the 129_hide_inputs@filter_panel_refactor@main branch February 13, 2023 16:08

lcd2yyz mentioned this pull request Mar 17, 2023

Filter card summary design #220

Closed

	private$varlabel <- if (identical(varlabel, as.character(varname))) {
	# to not display duplicated label
	character(0)
	} else {
	varlabel
	}

	private$add_filter_states(
	filter_states = init_filter_states(
	data = self$get_dataset(),
	input_dataname = as.name(dataname),
	output_dataname = as.name(dataname),
	varlabels = self$get_varlabels(),
	keys = self$get_keys()
	),
	id = "filter"
	)

	get_varlabels = function(variables = NULL) {
	checkmate::assert_character(variables, null.ok = TRUE, any.missing = FALSE)

	labels <- formatters::var_labels(private$dataset, fill = FALSE)
	if (is.null(labels)) {
	return(NULL)
	}
	if (!is.null(variables)) labels <- labels[intersect(self$get_varnames(), variables)]
	labels
	},

show/hide filter state inputs #181

show/hide filter state inputs #181

Conversation

asbates commented Jan 31, 2023

gogonzo 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

gogonzo Feb 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikolas-burkoff commented Feb 1, 2023

nikolas-burkoff commented Feb 1, 2023 • edited

donyunardi commented Feb 1, 2023 • edited

donyunardi commented Feb 1, 2023 • edited

asbates commented Feb 1, 2023

asbates commented Feb 1, 2023

donyunardi commented Feb 1, 2023

lcd2yyz commented Feb 1, 2023

gogonzo commented Feb 6, 2023

donyunardi 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcd2yyz commented Feb 6, 2023 • edited

gogonzo commented Feb 7, 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

lcd2yyz commented Feb 7, 2023

asbates commented Feb 7, 2023

asbates commented Feb 7, 2023

gogonzo commented Feb 7, 2023

gogonzo commented Feb 7, 2023 • edited

asbates commented Feb 7, 2023

nikolas-burkoff commented Feb 8, 2023

nikolas-burkoff commented Feb 8, 2023

lcd2yyz commented Feb 8, 2023 • edited

asbates commented Feb 8, 2023

gogonzo Feb 2, 2023 •

edited

nikolas-burkoff commented Feb 1, 2023 •

edited

donyunardi commented Feb 1, 2023 •

edited

donyunardi commented Feb 1, 2023 •

edited

lcd2yyz commented Feb 6, 2023 •

edited

gogonzo commented Feb 7, 2023 •

edited

lcd2yyz commented Feb 8, 2023 •

edited