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

7 previewer@main #33

Merged
merged 44 commits into from
May 12, 2022
Merged

7 previewer@main #33

merged 44 commits into from
May 12, 2022

Conversation

Polkas
Copy link
Contributor

@Polkas Polkas commented May 9, 2022

closes #7
closes #9
closes #19

I added a few new methods to ReportCard and Reporter.

jQuery + Shiny.setInputValue -> up/down/remove the previewer card

Accordion -> only one collapsible opened at the same time

The Tables are only printed to get the same output as Renderer, the formatting is persisted because of the pre html tag.

I have to add more tests and improve vignettes

Strange error to investigate (new issue) data.frame is not printed to the shiny output when run with green play button from them Rmd.

teal_reporter_previewer is a template for a function which have to be added later.

R/Previewer.R Outdated Show resolved Hide resolved
R/Previewer.R Outdated Show resolved Hide resolved
R/Previewer.R Outdated Show resolved Hide resolved
R/Previewer.R Outdated Show resolved Hide resolved
@Polkas Polkas marked this pull request as ready for review May 10, 2022 07:45
R/Previewer.R Outdated
Comment on lines 224 to 247
sprintf('
$(document).ready(function(event) {
$("body").on("click", "span.card_remove_id", function() {
var val = $(this).data("cardid");
let msg_confirm = "Do you really want to remove the card " + val + " from the Report?";
var answer = confirm(msg_confirm);
if (answer) {
Shiny.setInputValue("%s", val, {priority: "event"});
$("#panel_card_" + val).remove();
}
});

$("body").on("click", "span.card_up_id", function() {
var val = $(this).data("cardid");
Shiny.setInputValue("%s", val, {priority: "event"});
});

$("body").on("click", "span.card_down_id", function() {
var val = $(this).data("cardid");
Shiny.setInputValue("%s", val, {priority: "event"});
});
})
', ns("card_remove_id"), ns("card_up_id"), ns("card_down_id"))
))
Copy link
Contributor

Choose a reason for hiding this comment

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

I like idea of not creating observe for each remove/swap button!

@nikolas-burkoff
Copy link
Contributor

nikolas-burkoff commented May 10, 2022

It's looking good!

  • Should we allow the report to be downloaded when there are no cards added?
  • Should we add a reset reporter button in the previewer?
  • Not part of this PR but I can guarantee app users (not the app developers) will want to set these names :)

image

@Polkas
Copy link
Contributor Author

Polkas commented May 10, 2022

It's looking good!

  • Should we allow the report to be downloaded when there are no cards added?
  • Should we add a reset reporter button in the previewer?
  • Not part of this PR but I can guarantee app users (not the app developers) will want to set these names :)

image

THis card preview title is not a part of the report document, I do not think user want to customize it. This should be a proper name specified by the developer of the app who knows what It presents. More precisely a card preview title element is not a part of any end product here.

Btw the card view is built by the developer too.

@nikolas-burkoff
Copy link
Contributor

This card preview title is not a part of the report document, I do not think user want to customize it.

We shall see :)

@Polkas
Copy link
Contributor Author

Polkas commented May 10, 2022

This card preview title is not a part of the report document, I do not think user want to customize it.

We shall see :)

Sorry, we think of the TextInput for all TextBlock in the future. I think this will make the app much more complex, code lines and reactivity.

@Polkas
Copy link
Contributor Author

Polkas commented May 11, 2022

If we want to have disable option (when there are no cards) for download button in the previewer we have to apply:

  • renderUI of the Download button
  • add shinyjs dependency and use disable/enable functions
  • add an additional argument (reporter) to the previewer UI function

I do not like any of the options so I leave it as it is. If you really want to have it I will go with the shinyjs solution.

R/Previewer.R Outdated Show resolved Hide resolved
R/Previewer.R Outdated Show resolved Hide resolved
R/Previewer.R Outdated Show resolved Hide resolved
Copy link
Contributor

@mhallal1 mhallal1 left a comment

Choose a reason for hiding this comment

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

It would also be good to not be able to reset the report (similar to download button) when there are no cards:
Screenshot from 2022-05-12 12-59-08

@Polkas
Copy link
Contributor Author

Polkas commented May 12, 2022

It would also be good to not be able to reset the report (similar to download button) when there are no cards.

It will not be compatible with the simple previewer.
The reset button is a separate module now, if you want such functionality it have to be done there.

@mhallal1
Copy link
Contributor

mhallal1 commented May 12, 2022

It would also be good to not be able to reset the report (similar to download button) when there are no cards.

It will not be compatible with the simple previewer. The reset button is a separate module now, if you want such functionality it have to be done there.

fair enough, i will spin an issue for this task: #42

Copy link
Contributor

@mhallal1 mhallal1 left a comment

Choose a reason for hiding this comment

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

no more comments from my side and from @kpagacz .
Well done.

@Polkas Polkas merged commit 2ebdba7 into main May 12, 2022
@Polkas Polkas deleted the 7_previewer@main branch May 12, 2022 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants