Next Release

[Jeff-Thompson12]

@AARON-CLARK I think we are in a pretty good place here. A lot of improvements and enhancements. Is there anything I'm missing?

I haven't written any vignettes. The README is pretty expansive of the functionality. I suppose I should write up a vignette highlighting some of the new features.

Also, feel free to explore the updated filter on tidyCDISC.

[AARON-CLARK]

I haven't written any vignettes. The README is pretty expansive of the functionality. I suppose I should write up a vignette highlighting some of the new features.

HI @Jeff-Thompson12, I haven't reviewed this in it's entirety (yet), but yes, I would argue for having at least one vignette so we can get credit via riskmetric (#6).

[Jeff-Thompson12]

I will add a vignette but want to wait for your more complete feedback first.

[AARON-CLARK]

I love the new eraser option implemented recently. Specifically, that IDEAFilter keeps the variable selected, but allows the user to quickly toggle the filter off & restart. Genius!

Not a huge deal, but one thing I would suggest for the reviewer and our future selves is including more information in your PRs so it's easier to identify what the old PRs hoped to achieve - maybe throw in a screenshot. I think that's just "nice to have" so I (and future you) can quickly understand what each PR did/ does.

Also, examples on how to implement new arguments would be nice, although that info is probably planned for the vignette you want to compose. The column subsetting did not have an example to follow. I would advise adding examples on perfroming both "column subsetting" and "pre-selection" in one or both of the example apps in inst/, plus via the @examples tag in the roxygen2.

FYI, I did turn your tidyCDISC branch into an official PR here.

I was able screw some things up during my testing!

Conflicting col_subset & preselection

First, I was able to crash the STARWARS example app by pre-selecting a variable name below, but then removing it using the col_subset feature.

filtered_data <- IDEAFilter("data_filter",
                             data = starwars2,
                             preselection = list(name = list(filter_na = TRUE,
                                                             filter_fn = ~stringr::str_detect(.x, '[0-9]'))),
                             col_subset = c("height", "mass", "hair_color", "eye_color", "vehicles", "is_droid"),
                             verbose = FALSE)

Note, the app didn't crash until I clicked this little button:

The console just displayed:

Failed render when shiny vector type changes

This problem probably existed before your changes, so if you want to push it down the road, that's fine but this was the sequence of events that I tested to reproduce the issue:

For the STARWARS app, select one name and one height. Note that height is a quantitative measure which will show up as a range slider when there exists more data points... I think more than 5 or 8? I can't really remember.

So from here, I can do three things to make things go haywire:

1. de-select Ackbar
2. click the new eraser button
3. shift the height filter to position # 1

I'll do the latter. You can see that the name variable's filter item freaks out and the quantitative height variable adopts a < 180 approach.

I think the desired result is to have the quantitative filter stick to one number, as shown below:

I think exploring behaviors of these shifting UIs should be explored more deeply, since they exist for other data types as well, not just the quantitative range selectors.

[Jeff-Thompson12]

@AARON-CLARK Neither issue predates me.

1. Conflicting col_subset and preselection I definitely was the one to introduce this issue. I added code to destroy observers when removing a filter item or changing the column. I also did this for shiny_data_filter_item() because of changing made to vector modules. I think we can go one of two ways: (1) make sure the preselection column is in the subset of columns (easy fix) or (2) allow incorrect pre-selections and fix the bug causing the app to crash. My vote is for option 2 because there exists the concept of a NULL column type. This also handles the issue of the pre-selection failing silently.
2. Failed render when shiny vector type changes Fairly straightforward fix. The issue I found was that the input$param is being shared. I added a function remove_shiny_inputs() that will take care of this issue when switching the column. This is the only instance were the data type can change in a meaningful way that impacts input$param (the inputs for factor few and many are interchangeable). I will push a commit to address this.

[AARON-CLARK]

1. My vote is for option 2

I was just thinking option 1, but if option 2 floats your boat, your boat should float 👍🏼.

[AARON-CLARK]

Per our discussion today, I found this little issue, likely having to do with the merge between ADSL & ADLB. This exists on tidyCDISC's master branch too.

.

[AARON-CLARK]

@Jeff-Thompson12, was there anything left to do for the next release besides adding the vignette (#29)?

[Jeff-Thompson12]

I think that was it. I have a local branch that isn't finished where I was working on that. There were a few changes that I made on that branch as well. I will open a PR for it.

Oops. PR was already opened. #29

[Jeff-Thompson12]

@AARON-CLARK I think we are there. If you want to take a look at the changes or have any additional checks or edits.

Also, I updated by contribution level from contributor to author. Seemed appropriate to me.