Skip to content
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

133 take 2@filter panel refactor@main #268

Merged

Conversation

asbates
Copy link
Contributor

@asbates asbates commented May 5, 2023

Adds a numeric range input to range filter state cards so range can be set manually. This is a redo of #226

Fixes #133

@asbates asbates added the core label May 5, 2023
Copy link
Contributor

@chlebowa chlebowa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid inputs cannot crash the whole app. There has to be some way to block that error. shinyvalidate is one option but I don't know whether we want to add it to teal.slice.

@gogonzo
Copy link
Contributor

gogonzo commented May 9, 2023

Invalid inputs cannot crash the whole app. There has to be some way to block that error. shinyvalidate is one option but I don't know whether we want to add it to teal.slice.

We can't use shinyvalidate as FilterPanel must not return any shiny-validate errors - this would have a serious consequences downstream in teal and teal_module. So, if user sets the wrong value it should be automatically set to the latest selected with notification.

R/FilterStateRange.R Outdated Show resolved Hide resolved
Copy link
Contributor

@chlebowa chlebowa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Numeric selection states on app start do not follow api specification:

default_filters <- filter_settings(
  filter_var("iris", "Sepal.Length", sel = c(5.1, 6.4), fixed = FALSE),
  filter_var("iris", "Sepal.Width", sel = c(2.5, 4.0), fixed = FALSE, disabled = TRUE),
  filter_var("iris", "Petal.Length", sel = c(4.5, 5.1), fixed = TRUE),
  filter_var("iris", "Petal.Width", sel = c(0.3, 1.8), fixed = TRUE, disabled = TRUE),
  filter_var("iris", "Species", sel = c("versicolor", "virginica"), fixed = FALSE),
  filter_var("iris", "Species2", sel = c("versicolor", "virginica"), fixed = TRUE)
)

image

Also, choices become limited.

@chlebowa
Copy link
Contributor

chlebowa commented May 9, 2023

I think RangeFilterState$set_choices is broken but that's a different issue. I will look into it a little more.

@chlebowa
Copy link
Contributor

chlebowa commented May 9, 2023

Numeric selection states on app start do not follow api specification:
(...)

My mistake, the numbers in the app are the actual data ranges.

@chlebowa chlebowa self-requested a review May 9, 2023 16:15
@asbates
Copy link
Contributor Author

asbates commented May 9, 2023

Numeric selection states on app start do not follow api specification:

default_filters <- filter_settings(
  filter_var("iris", "Sepal.Length", sel = c(5.1, 6.4), fixed = FALSE),
  filter_var("iris", "Sepal.Width", sel = c(2.5, 4.0), fixed = FALSE, disabled = TRUE),
  filter_var("iris", "Petal.Length", sel = c(4.5, 5.1), fixed = TRUE),
  filter_var("iris", "Petal.Width", sel = c(0.3, 1.8), fixed = TRUE, disabled = TRUE),
  filter_var("iris", "Species", sel = c("versicolor", "virginica"), fixed = FALSE),
  filter_var("iris", "Species2", sel = c("versicolor", "virginica"), fixed = TRUE)
)

image

Also, choices become limited.

Huh, it's working for me. This is what I get on launch.

image

And after enabling

image

@chlebowa
Copy link
Contributor

chlebowa commented May 9, 2023

Huh, it's working for me. This is what I get on launch.

image

uh-oh 🤔

@donyunardi
Copy link
Contributor

I have the same result with @asbates.
This is what I see upon launch:

@chlebowa
Copy link
Contributor

chlebowa commented May 9, 2023

All right, my session was littered and another object got pulled into the app. Works fine for me as well.

Copy link
Contributor

@donyunardi donyunardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@asbates asbates merged commit cb82947 into filter_panel_refactor@main May 9, 2023
@asbates asbates deleted the 133_take_2@filter_panel_refactor@main branch May 9, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants