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

Conversation

@alcinnz
Copy link

@alcinnz alcinnz commented Oct 18, 2019

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
Copy link
Owner

@cassidyjames cassidyjames left a comment

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.

*
* Authored by: Adrian Cochrane <[email protected]>
*/
namespace Ephemeral {
Copy link
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
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.

@cassidyjames
Copy link
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 <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
Co-Authored-By: Cassidy James Blaede <[email protected]>
@alcinnz
Copy link
Author

alcinnz commented Oct 18, 2019

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