Skip to content
This repository was archived by the owner on Apr 2, 2022. It is now read-only.

Implement downloads#201

Open
alcinnz wants to merge 26 commits into
cassidyjames:mainfrom
alcinnz:master
Open

Implement downloads#201
alcinnz wants to merge 26 commits into
cassidyjames:mainfrom
alcinnz:master

Conversation

@alcinnz

@alcinnz alcinnz commented Oct 18, 2019

Copy link
Copy Markdown

Fixes #36

It matches your mockup, except it uses a ListBox for better keyboard input and nicer layout upon removing a download. And as requested it shows a Toast once the download has been completed.

@cassidyjames cassidyjames changed the title Fix issue #36 Implement downloads Oct 18, 2019
@cassidyjames cassidyjames added this to the 7.0 milestone Oct 18, 2019

@cassidyjames cassidyjames left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Awesome work! I did a quick pass over code style and am going to dogfood this for a while, but left some quick notes, mostly around indentation/whitespace.

Comment thread src/Widgets/DownloadsButton.vala
Comment thread src/Widgets/DownloadsButton.vala Outdated
*
* Authored by: Adrian Cochrane <alcinnz@lavabit.com>
*/
namespace Ephemeral {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I typically avoid the extra indentation level and just namespace the class itself, i.e. Ephemeral.DownloadsButton. I think I'd like to do that here as well for consistency with the rest of the codebase.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I differed for the sake of two helper classes, but I can make that adjustment.

Comment thread src/Widgets/DownloadsButton.vala Outdated
Comment thread src/Widgets/DownloadsButton.vala Outdated
Comment thread src/Widgets/DownloadsButton.vala Outdated
Comment thread src/MainWindow.vala Outdated
Comment thread src/MainWindow.vala Outdated
Comment thread src/Widgets/DownloadsButton.vala Outdated
Comment thread src/Widgets/DownloadsButton.vala Outdated
Comment thread src/Widgets/DownloadsButton.vala Outdated
Comment thread src/Widgets/DownloadsButton.vala Outdated
@cassidyjames

Copy link
Copy Markdown
Owner

It looks like a lot of the margins and whatnot didn't get carried over from the prototype branch as well. I can work on cleaning those up, but I just thought I'd share here for posterity.

Prototype

Screenshot from 2019-10-17 23 44 31

This branch

Screenshot from 2019-10-17 23 43 45

alcinnz and others added 17 commits October 19, 2019 07:46
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
Co-Authored-By: Cassidy James Blaede <cassidy@elementary.io>
@alcinnz

alcinnz commented Oct 18, 2019

Copy link
Copy Markdown
Author

I've gone over your feedback and incorporated it into my pull request.

@alcinnz alcinnz requested a review from cassidyjames October 18, 2019 19:02
@cassidyjames cassidyjames changed the base branch from master to main June 21, 2020 21:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support Downloads

2 participants