lockedata / starters

R Package πŸ“¦ for initializing projects for various R activities :nut_and_bolt:
https://itsalocke.com/starters/
GNU General Public License v3.0
124 stars 16 forks source link

Repair RStudio add-in #101

Closed jonmcalder closed 5 years ago

jonmcalder commented 5 years ago

To address #71

projects-add-in

Hopefully this will make the add-in functional again following all the breaking changes introduced earlier in the year.

I imagine this will need some review and iteration (and is subject to outstanding changes such as those in #100 which I am assuming will be accepted), so it's WIP for now.

Working on this seems to keep highlighting little issues - which I suppose is a good thing but rather frustrating, since these are tangential and interrupt progress, especially when only looking at it every now and again in one's spare time. Anyway, I felt I just had to open this PR before the end of the year since I first agreed to fix the add-in 2 months ago πŸ™ˆ

I've changed to the modal dialog viewer in order to facilitate a bigger and better UI/layout. This is also required due to the introduction of shinyFiles (for selecting the project folder) which is not really useable within the smaller miniUI layouts (particularly within an RStudio pane) without having to scroll vertically.

projects-add-in-select-folder

The downside of this is that the modal stays open and partially obscures the terminal (after clicking done) until whichever createProject() function was selected has finished running. Due to the interactive nature of usethis, one usually needs to confirm or make a few terminal selections as the functions are executed, so one has to manually close the shiny modal in order to do so.

Can anyone think of a workaround for this? We could try sticking to the smaller UI within the RStudio viewer pane, but it will then require some vertical scrolling to use since the input widgets (especially for shinyFiles) can't really be forced into such a narrow / small space (example shown below).

projects-viewer-pane

As always am happy to accept any feedback, criticism, or suggestions on this.

maelle commented 5 years ago

Thanks a ton for all this work! And thanks for discovering issues whilst doing so!

maelle commented 5 years ago

Or a makeshift solution: the app could simply output the function call as string, with a button to copy it to the clipboard, and the user would paste it in the terminal themselves?

jonmcalder commented 5 years ago

Thanks a ton for all this work! And thanks for discovering issues whilst doing so!

Thanks - happy to be able to help!

  • @stephlocke do you have any tip regarding Shiny? usethis functions ask stuff depending on usethis:::interactive() and I'm not sure whether one could manipulate it to return FALSE when the functions are launched from Shiny πŸ€” Or can one change the layout "reactively"?

My sense is that while one or more of the above suggestions are probably possible, they might not be worth the maintenance burden they could introduce (not to mention the potential complication introduced when trying to assist people with debugging issues etc). In my opinion the code/process flow after hitting "Done" on the shiny gadget should be exactly the same (or as similar as possible) as when calling the functions from the console (with the same values for arguments).

  • Does any of you know whether one could add details about each option on hovering? E.g. "Reset Project" might look mysterious.

Yes - tooltips can be added simply by wrapping the inputs in a div and adding a title. See example below.

screen shot 2019-01-03 at 20 44 00
jonmcalder commented 5 years ago

Or a makeshift solution: the app could simply output the function call as string, with a button to copy it to the clipboard, and the user would paste it in the terminal themselves?

This might actually be ok as a workaround. It has the advantage of facilitating a final check of all argument values before committing to execution of the function call.

stephlocke commented 5 years ago

I'm happy for the gui to stay in the gadget pane rather than overlay rstudio - the vertical scroll doesn't look too bad

jonmcalder commented 5 years ago

My main concern with the vertical scroll is not so much that it looks bad, but more so from a UX point of view because the action buttons (e.g. in the case of choosing the project folder with shinyFiles) are not visible and not that easily discoverable unless you think to scroll.

stephlocke commented 5 years ago

Fair enough - either progress with it or use the standard file picker. I'm happy either way :)

maelle commented 5 years ago

Noting stuff as I go.

Could we use whoami to fill the login of the external setup by default? Or is it too GitHub oriented to do that?

maelle commented 5 years ago

but we only support GitHub at the moment so :nail_care:

jonmcalder commented 5 years ago

Noting stuff as I go.

Could we use whoami to fill the login of the external setup by default? Or is it too GitHub oriented to do that?

I'm guessing you missed line 62? πŸ˜„

maelle commented 5 years ago

oh I need to update my setup then!!

maelle commented 5 years ago

I have not played a ton with the addin but generally find it very nifty. Thanks a ton!

Have you added tooltips to explain parameters or not yet? I have not seen them but I might have missed that. Anyhow I can help adding them if needed.

jonmcalder commented 5 years ago

I have not played a ton with the addin but generally find it very nifty. Thanks a ton!

Glad to hear that - thank you! I have found myself wondering whether creating a 'GUI' like this is worth it given the likely target audience for this package, but your feedback suggests that maybe it does have it's place.

Have you added tooltips to explain parameters or not yet? I have not seen them but I might have missed that. Anyhow I can help adding them if needed.

I only added the tooltip to the Reset Project checkbox for now since that was the one you mentioned as a potential concern. Happy to add more - or for you to do so.

Thanks again for the review!

maelle commented 5 years ago

I think tooltips might be good for name and title.

Another thought, could the addin message the starters function call for users to save it for re-use?

jonmcalder commented 5 years ago

I think tooltips might be good for name and title.

Ok sure.

Another thought, could the addin message the starters function call for users to save it for re-use?

Yes - although message output may get a little lost due to all the subsequent message output from usethis as the functions get executed.

Maybe outputting as a message and also copying to the clipboard using clipr?

maelle commented 5 years ago

Maybe outputting as a message and also copying to the clipboard using clipr?

Good idea! Beside that and the tooltips to add, I'm ok with this PR being merged, awesome work!

codecov-io commented 5 years ago

Codecov Report

Merging #101 into master will decrease coverage by 12.23%. The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #101       +/-   ##
===========================================
- Coverage   56.96%   44.72%   -12.24%     
===========================================
  Files          12       12               
  Lines         625      796      +171     
===========================================
  Hits          356      356               
- Misses        269      440      +171
Impacted Files Coverage Ξ”
R/utils.R 34.11% <0%> (-9.17%) :arrow_down:
R/shinyGadget.R 0% <0%> (ΓΈ) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update b85140f...6570330. Read the comment docs.

maelle commented 5 years ago

I like that this PR is called "Repair the addin" but actually also takes it to a new level, awesome work @jonmcalder!

:star2: The starters function call is shown below and has been copied to the clipboard: :star2: so cool!

Thanks a ton!