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

Dependency "Upload All" follow up#626 #628

Merged
merged 4 commits into from
Aug 17, 2023
Merged

Dependency "Upload All" follow up#626 #628

merged 4 commits into from
Aug 17, 2023

Conversation

Eduardodudu
Copy link
Collaborator

Task

Feature task on the option to hide upload all packages when there is no package to be shown in table #626

This PR adds

  1. Checks if there are packages to be rendered on the table and only shows upload all option when it does

image

@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Merging #628 (995f766) into dev (60f0c0c) will increase coverage by 0.19%.
Report is 14 commits behind head on dev.
The diff coverage is 14.81%.

❗ Current head 995f766 differs from pull request most recent head 0679c75. Consider uploading reports for the commit 0679c75 to get more accurate results

@@            Coverage Diff             @@
##              dev     #628      +/-   ##
==========================================
+ Coverage   76.44%   76.64%   +0.19%     
==========================================
  Files          29       29              
  Lines        3919     3917       -2     
==========================================
+ Hits         2996     3002       +6     
+ Misses        923      915       -8     
Files Changed Coverage Δ
R/mod_packageDependencies.R 18.66% <14.81%> (+2.80%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Eduardodudu Eduardodudu marked this pull request as ready for review August 17, 2023 13:52
Copy link
Collaborator

@AARON-CLARK AARON-CLARK left a comment

Choose a reason for hiding this comment

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

Hi @Eduardodudu, I know I didn't specify this request in the #626, but I think it'd also be valuable to have the "Upload All" button be hidden when there are no packages to upload too (see use case below). To me, it makes sense to tackle this change in this PR since it's highly related to checking for rows in the table and you already wrote the logic in #616. Specifically, your authored this logic which is very handy:

pkgs_update <- pkg_df() %>%
dplyr::filter(is.na(score) | score == "") %>%
dplyr::filter(!name %in% c(rownames(installed.packages(priority = "base"))))
n_packages <- nrow(pkgs_update)
if (n_packages > 0) {

Here is a use case. I already have the zoo package uploaded, so I don't need the option to "Upload All":

image

@Eduardodudu
Copy link
Collaborator Author

Hi @Eduardodudu, I know I didn't specify this request in the #626, but I think it'd also be valuable to have the "Upload All" button be hidden when there are no packages to upload too (see use case below). To me, it makes sense to tackle this change in this PR since it's highly related to checking for rows in the table and you already wrote the logic in #616. Specifically, your authored this logic which is very handy:

pkgs_update <- pkg_df() %>%
dplyr::filter(is.na(score) | score == "") %>%
dplyr::filter(!name %in% c(rownames(installed.packages(priority = "base"))))
n_packages <- nrow(pkgs_update)
if (n_packages > 0) {

Here is a use case. I already have the zoo package uploaded, so I don't need the option to "Upload All":

image

Hi @AARON-CLARK!

I actually enjoyed a lot this idea!

Right now the app will only show "Upload All" Icon when there is actually a package to be uploaded.

@AARON-CLARK AARON-CLARK self-requested a review August 17, 2023 17:35
Copy link
Collaborator

@AARON-CLARK AARON-CLARK left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Eduardodudu Eduardodudu merged commit e0d2949 into dev Aug 17, 2023
3 checks passed
@AARON-CLARK AARON-CLARK deleted the ea-626 branch August 29, 2023 14:31
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.

2 participants