-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #400 Download popup for Android browser #663
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
Fix #400 Download popup for Android browser #663
Conversation
|
Thanks for a great contribution @jainsahab. We'll need to make some small icon / UI tweaks so I'll speak to our product team and get back to you with details as well as a PR review. |
|
Sure @subsymbolic , Thanks a lot for acknowledgement. Let me know whenever you have mock-ups and feedback on code ready. |
app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt
Outdated
Show resolved
Hide resolved
|
Hi @subsymbolic , Just wanted to know about the status of this PR. are UI and icons finalized ? Thanks, |
|
Hi @jainsahab, hopefully our product team will have some time to look at his next week. |
|
Thank you so much for you patience, we now have a design for you 🎉 Check out The new vector assets are available at https://gist.github.com/subsymbolic/f16d4c275bb871fdfca0424307b2f525. Let me know if you have any questions! |
|
Hi @subsymbolic , I am done with my changes, Let me know if you have any feedback. |
|
Hi @subsymbolic , could you please have a look at this PR, otherwise we might get more conflicts and the PR would need more work. |
|
Definitely! Apologies for the delay, I have time to push this forward from Wednesday to Friday this week. |
|
Hi @subsymbolic , I've resolved the conflicts as of now. Let me know about your feedback when you get hold of it. |
subsymbolic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for you patience, I now have the time to give this the attention it needs! This is looking great, getting really close now. I have added my feedback inline.
app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt
Outdated
Show resolved
Hide resolved
|
Hi @subsymbolic , Thanks for reviewing the code. I have incorporated all the feedback mentioned. |
|
Hi @subsymbolic , I really appreciate your efforts to take time out of your busy schedule and reviewing community's PR like this. Could you please finish this PR also, I are already done with the feedback and have resolved conflicts two times since then. Thanks, |
subsymbolic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great, I have sent it over to our product team for approval. There are a few final small PR comments, once they are done and we have product approval we can merge this.
app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/downloader/NetworkFileDownloadManager.kt
Outdated
Show resolved
Hide resolved
1edeac5
into
duckduckgo:feature/multiple_authors/download_confirmation
|
The code looks good, great work 🎉 Our product team has also come back and is happy to proceed however they don't want to show this dialog when we have already shown our own confirmation, such as when a user long-presses and requests to download an image. As this is a new request, I am accepting your PR in a feature branch and once we have implemented the newly requested dialog behavior, we will release both together. Thanks for all your work on this 🙏 |


Task/Issue URL: #400
Tech Design URL:
CC: @subsymbolic @marcosholgado @CDRussell @christophergeiger3
Description:
After this change, whenever browser user is initiating any download, a confirm popup will appear on the screen to confirm or deny the download. If the same file is already downloaded, the user will be presented with options like Keep both, Replace and Open with to open an existing file and abandon the download.
In the second iteration, we can also provide an option to the user where he/she can provide a custom name for the downloaded file.
Steps to test this PR:
Internal references:
Software Engineering Expectations
Technical Design Template