Closed chlebowa closed 1 month ago
Hey @chlebowa thanks for the input! I think it's gonna be highly useful!
I allowed CI check to be executed on this PR. I see a Warning in R CMD Check - would you be able to check in your free time?
Thanks @m7pr, I had forgotten to re-create documentation after removing the faulty defaults.
Yeah, this is something @walkowif is investing on our end
The urlchecker
failure should be gone after retriggering the checks.
Thank you @chlebowa for the time and energy you invested to provide this feature. I just test in on the branched cloned from your fork. Works like a charm! I think we are good to merge this excellent feature, once we figure out the details from the comments.
@kumamiao would you mind providing a NEWS entry for this feature?
https://github.com/insightsengineering/teal/assets/133694481/ca98b4af-f994-48c7-a796-f37de1ad49da
I had a thought this morning that may be a slightly more orderly implementation. Rather than creating tro Progress
objects, one in a reactive expression and one in an observer, instantiate one Progress
in srv_teal
and work with it throughout the function by first passing it to modules_datasets
and later reset the progress and pass to ui_tabs_with_filters
.
The user would see only one progress bar, whereas now two bars can be seen, the filter one is moved up and later disappears as the module one appears. On the code seide, there would be a little less clutter.
What do you think?
If one waits for the other, then it's probably better for the end user to see just one progress bar at a time :) Depending on how much time you have. This is already in good condition
Have a look.
Wow @chlebowa nice and clean. Really ready to be merged!
Can you
__stres_test.R
file* Provided progress bar for modules loading and data filtering during teal app startup.
Then I will merge. Thank you. Really great job!
Hi all. Apologies for jumping into the old PR - just got back from the break of mine. May I ask for a follow-up PR before the release that will make progress
argument optional for all modified functions? The rationale here is simplicity - we would like to keep the requirements to run the funcs to the meaningful minimum which is in these case: id
, modules
and datasets
. If you look at the functions definitions all the other arguments (incl. filter
which is also quite important) has some defaults. In case of progress
this could be NULL
and if it's Progress
we will increment it. Another benefits are in testing - no need to create a dummy progress object to execute functions.
(*) I know the is.missing()
stuff but IMHO having x = NULL
in the function definition exposes this better without reading docs or function body
@pawelru I created a ticket for that in here https://github.com/insightsengineering/teal/issues/1243
Related to #1234.
Update progress bars to improve user experience.
modules_datasets
receives optionalprogress
argument that accepts aProgress
objectui_nested_tabs
andui_tabs_with_filters
receiveprogress
argument that accepts aProgress
objectsrv_teal
Added a large app for testing in
__stress_test.R
, to be removed before merging.NOTE
Once the module UI is prepared, there is still a delay before the app starts, so the progress bar can be improved further. So far I was unable to identify the reason for the delay.