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

4051 download all files #4363

Merged
merged 15 commits into from
Dec 19, 2017
Merged

4051 download all files #4363

merged 15 commits into from
Dec 19, 2017

Conversation

sekmiller
Copy link
Contributor

New Contributors

Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!

Related Issues

Pull Request Checklist

  • Unit tests None
  • Integration testsx]: None
  • Deployment requirements: None
  • DocumentationN/A
  • Merged latest from "develop" branchx] and resolved conflicts

public void toggleSelectedFiles(){
//method for when user clicks (de-)select all files
public void toggleAllSelected(){
System.out.print("in toggle all");
Copy link
Member

Choose a reason for hiding this comment

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

Can you please remove this System.out.print or convert it to logger.fine if it might be valuable when debugging in production?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 12.706% when pulling 6a4691e on 4051-download-all-files into ef7dce7 on develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 12.706% when pulling 22a5fac on 4051-download-all-files into ef7dce7 on develop.

Copy link
Member

@pdurbin pdurbin 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.

public void toggleSelectedFiles(){
//method for when user clicks (de-)select all files
public void toggleAllSelected(){
this.selectAllFiles = !this.selectAllFiles;
Copy link
Member

Choose a reason for hiding this comment

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

At first blush this.selectAllFiles = !this.selectAllFiles was confusing to me but I understand that this is a toggle. Clicking means "reverse the state of this thing". We could add a comment to clarify this.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 12.705% when pulling 294a1dd on 4051-download-all-files into ef7dce7 on develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 12.851% when pulling dd88232 on 4051-download-all-files into 2c2d9c8 on develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 12.84% when pulling 2d6e2c3 on 4051-download-all-files into 2c2d9c8 on develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 12.84% when pulling 8952213 on 4051-download-all-files into 2c2d9c8 on develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 12.84% when pulling b3bcdc9 on 4051-download-all-files into 80e75b6 on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 12.846% when pulling c5a2da6 on 4051-download-all-files into 80e75b6 on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 12.844% when pulling c7cf82c on 4051-download-all-files into 80e75b6 on develop.

@kcondon kcondon merged commit 530377d into develop Dec 19, 2017
@kcondon kcondon deleted the 4051-download-all-files branch December 19, 2017 21:16
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.

5 participants