Skip to content

Fix for issue #1037 #1086

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

Closed
wants to merge 3 commits into from
Closed

Fix for issue #1037 #1086

wants to merge 3 commits into from

Conversation

mithlesh4257
Copy link

Fix for issue #1037
Explicitly show that the license text is a link.

@psh, @misaochan Kindly review it.

@codecov-io
Copy link

Codecov Report

Merging #1086 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1086   +/-   ##
======================================
  Coverage    4.11%   4.11%           
======================================
  Files         114     114           
  Lines        5298    5298           
  Branches      501     501           
======================================
  Hits          218     218           
  Misses       5065    5065           
  Partials       15      15
Impacted Files Coverage Δ
.../free/nrw/commons/upload/SingleUploadFragment.java 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 b21d1e3...0d5ede1. Read the comment docs.

@maskaravivek
Copy link
Member

@mithlesh4257 Any particular reason for opening a new PR for the same issue?

IMO the discussions get lost if the same PR is not maintained.

@mithlesh4257
Copy link
Author

Actually @maskaravivek, I don't know how to manage this huge repository multiple file. What I mean to say I don't know how to add another commit to same PR.

@maskaravivek
Copy link
Member

After committing your changes locally, just do a git push <branch_name>. It will automatically add that commit to the PR. :)

@mithlesh4257
Copy link
Author

mithlesh4257 commented Jan 22, 2018

Thanks @maskaravivek. Now is this PR according?

import timber.log.Timber;

import static android.view.MotionEvent.ACTION_DOWN;
import static android.view.MotionEvent.ACTION_UP;

public class SingleUploadFragment extends CommonsDaggerSupportFragment {
public class SingleUploadFragment extends DaggerFragment {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you changed this?

Copy link
Author

Choose a reason for hiding this comment

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

I haven't changed it, it's automatically changed.

Copy link
Member

Choose a reason for hiding this comment

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

Please undo it. Usage of DaggerFragment would cause crashes.

Copy link
Author

Choose a reason for hiding this comment

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

Sure I will remove it @maskaravivek

Copy link
Author

@mithlesh4257 mithlesh4257 Jan 24, 2018

Choose a reason for hiding this comment

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

@maskaravivek, I had deleted the previous Commons forked repository due to some bugs and forked again. Can I make another PR as through this I'm unable to change my files? Please help @maskaravivek !

Copy link
Author

Choose a reason for hiding this comment

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

Should I make another PR that is in that I just have changed "upload/SingleUploadFragment.java" file as I had deleted the previously forked repo, I'm not also able to change this file directly from this PR.
@maskaravivek, @neslihanturan Please help!

Copy link
Author

Choose a reason for hiding this comment

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

Or I have one solution for it, May you edit this @maskaravivek , @neslihanturan?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mithlesh4257 you can fork it again, and make a fresh PR don't worry. I am closing this. Please follow this document before https://github.com/commons-app/apps-android-commons/wiki/Setting-up-dev-enviroment by changing "maskaravivek" username with yours. And please make sure all of the steps we discussed here are covered before preparing PR. Make sure you checked your commits and added meaningful commit messages before pushing.

Besides, if you want to leave this task for someone else to handle, please notify us.




*/

Copy link
Collaborator

Choose a reason for hiding this comment

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

For what this empty lines stands for?

Copy link
Author

Choose a reason for hiding this comment

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

I'm removing it.

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