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

change default argument reactive(NULL) #180

Conversation

nikolas-burkoff
Copy link
Contributor

@nikolas-burkoff nikolas-burkoff commented Jan 31, 2023

Closes #178

  • I prefer reactive(NULL) than NULL
  • We are limited in what we do in initialize as if we check the value of x_reactive then we need isolate around the constructor
  • I'm open to suggestions regarding making this nicer
  • Added the css into the filterstates ui as well as we need it for the RangeFilterState histograms

Default of FilterStates is also reactive(NULL)

@nikolas-burkoff
Copy link
Contributor Author

I guess if we change from reactive(NULL) to just NULL we can do something smarter in initialize as then the check if(is.null(x_reactive)) doesn't need an isolate...

@gogonzo
Copy link
Contributor

gogonzo commented Feb 1, 2023

I think these get_choices_label functions are too complicated. I think we can simplify and tidy up the way how we obtain counts. In the init of choicesFilterState there is private$histogram_data which isn't used at all. Having private$choices where value is a factor level and name is a count is also not elegant. I think we can introduce

private$choices # factor levels
private$choices_count # unfiltered count
private$f_choices_count # reactive
private$calculate_choices_count(x) 

That is just a proposition, I think it would be nice if we tidy up these initialize methods and how we preserve counts.

Issue #183

Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

👍 Thanks

@nikolas-burkoff nikolas-burkoff merged commit 1e6a91c into filter_panel_refactor@main Feb 3, 2023
@nikolas-burkoff nikolas-burkoff deleted the 178_xreactive_@filter_panel_refactor@main branch February 3, 2023 15:05
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.

None yet

2 participants