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

Repair RStudio add-in #101

Merged
merged 5 commits into from
Feb 11, 2019
Merged

Repair RStudio add-in #101

merged 5 commits into from
Feb 11, 2019

Conversation

jonmcalder
Copy link
Contributor

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
Copy link
Contributor

maelle commented Jan 3, 2019

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

  • @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"?

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

@maelle
Copy link
Contributor

maelle commented Jan 3, 2019

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?

R/shinyGadget.R Show resolved Hide resolved
@jonmcalder
Copy link
Contributor Author

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
Copy link
Contributor Author

jonmcalder commented Jan 3, 2019

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@jonmcalder jonmcalder changed the title WIP: Repair RStudio add-in Repair RStudio add-in Jan 7, 2019
@jonmcalder jonmcalder requested a review from maelle January 8, 2019 14:19
@maelle
Copy link
Contributor

maelle commented Jan 10, 2019

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
Copy link
Contributor

maelle commented Jan 10, 2019

but we only support GitHub at the moment so 💅

DESCRIPTION Show resolved Hide resolved
@jonmcalder
Copy link
Contributor Author

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
Copy link
Contributor

maelle commented Jan 10, 2019

oh I need to update my setup then!!

@maelle
Copy link
Contributor

maelle commented Jan 10, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor

maelle commented Jan 10, 2019

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
Copy link
Contributor Author

jonmcalder commented Jan 10, 2019

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
Copy link
Contributor

maelle commented Jan 25, 2019

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
Copy link

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%) ⬇️
R/shinyGadget.R 0% <0%> (ø) ⬆️

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
Copy link
Contributor

maelle commented Feb 11, 2019

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

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

Thanks a ton!

@maelle maelle merged commit 18f0f83 into lockedata:master Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants