Added default download path #7023

NejcZdovc · 2017-02-03T10:40:56Z

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

Resolves #2110

Auditors

Test Plan

go to the preferences, general tab
define your default download path
toggle show dialog
based on your settings, download dialog should be opened or not
if you set to hide download dialog, files should be downloaded automatically to your default download path

NejcZdovc · 2017-02-03T10:45:32Z

@bradleyrichter in #2110 you added pencil at the end of the input. Can you please provide an icon for it, if we will use it. For now I set input to read only and when you click on it, dialog is shown automatically.

bsclifton

Two things I think:

@bradleyrichter can we get an icon for editing the path? It feels a little naked without it. Here's a demo:
@NejcZdovc Can we get some unit tests? Basically, it would be using the Enzyme library and extending our existing preferences tests to include these two new settings 😄

NejcZdovc · 2017-02-09T21:06:04Z

@bsclifton Sorry I added you as a review too soon. My mistake. This PR is not ready for a final review as you notice there is no test for this new feature. I added label ready-for-review so that @bradleyrichter can look through it and prepare icon for browse/edit. And yes I agree we need to have a browse/edit icon.

bradleyrichter · 2017-02-09T22:52:56Z

@NejcZdovc SVG icon for the edit (pencil) button attached:

browser_icon_pencil.svg.zip

bsclifton · 2017-02-16T20:49:22Z

@NejcZdovc was this one done? (or still needing some work?)

NejcZdovc · 2017-03-01T11:25:37Z

@bsclifton I added test's as well, so we are ready for a review

bsclifton

Comment left; also added a commit for you to check out 😄 If you rework the action, LMK and I can re-review. Otherwise, you should be good to squash it and this'll be ready for merge

bsclifton · 2017-03-03T08:00:41Z

app/browser/reducers/downloadsReducer.js

+        properties: ['openDirectory']
+      }, (folder) => {
+        if (Array.isArray(folder) && fs.lstatSync(folder[0]).isDirectory()) {
+          appActions.changeSetting(settings.DOWNLOAD_DEFAULT_PATH, folder[0])


I think this is fine as-is... but it could be better as it's own discreet action (in the event that we'd like other reducers to be listening). Perhaps an action like defaultDownloadPathChanged

bsclifton

actually- a few things I just noticed (was too focused on the code). Can you touch these up?

The pencil icon should probably change color when hovered. You should be able to look at other pages for an example (see the bookmarks manager for example).
The cursor for the pencil should be pointer I think, to be consistent with other controls. EDIT: it's OK as-is
You can also add a -webkit-user-select: none; to prevent the text cursor icon from showing up when the mouse either gets near it or page is click and dragged (and mouse goes near pencil icon) EDIT: to be addressed after Refactor of paymentTab #7481 is merged

NejcZdovc · 2017-03-08T10:50:02Z

@bsclifton First of all sorry, I somehow over looked your second review.

I added hover color effect. Regarding the pointer I remember talking with @bradleyrichter about it for bookmark manager and he said that we should use only color hover and not a pointer. Bookmark manager for example don't have single pointer hover, only color hover. I agree that we should be consistent, so we need a guide line for it.

For user select I suggest to add it after #7481 is merged, because I added aphrodite param for component SettingItem.

bsclifton

@NejcZdovc I think the approach you're recommending is reasonable 😄

I did notice a problem. When testing on Windows, I noticed that the "Save As..." functionality no longer works. When choosing "Save As...", it should always prompt the user... since they are going to pick the filename to save

When I picked the "Save As" option, it immediately did the download without prompting ☹️

NejcZdovc · 2017-03-10T21:32:48Z

Just for a reference, @bridiver added flag to handle "Save as" calls brave/muon@de90c69

bridiver · 2017-03-10T21:39:36Z

js/about/aboutActions.js

+  /**
+   * Open dialog for default download path setting
+   */
+  defaultDownloadPath: function () {


these no longer have to go in aboutActions and this file should be deprecated. Use appActions for now instead

bridiver · 2017-03-10T21:40:12Z

this will need changes to correspond with my muon updates to correctly handle the prompt

@bradleyrichter

Resolves brave#2110 Auditors: @bradleyrichter @bsclifton Test Plan: - go to preferences, general tab - define your default download path - toggle show dialog - based on your settings, download dialog should be opened or not - if you set don't show dialog, filse should be downloaded to your default set path # Added hover effect

NejcZdovc · 2017-03-16T00:56:55Z

@bsclifton added save prompt for save as. Ready for a review

bsclifton · 2017-03-16T22:00:44Z

An area we can improve (cc: @bradleyrichter):

If I have a long path, it gets clipped

It looks the exact same in Chrome though, so not a deal-breaker or anything... just an area we could do better 😄

bsclifton

Just tested changes out w/ the latest Muon prebuilt; things worked great 😄 ++++++

NejcZdovc added this to the 0.13.3 milestone Feb 3, 2017

NejcZdovc self-assigned this Feb 3, 2017

NejcZdovc requested review from bsclifton and bradleyrichter February 3, 2017 10:40

NejcZdovc added the settings label Feb 3, 2017

NejcZdovc force-pushed the feature/#2110-download branch from 4aebac9 to 4ac876a Compare February 3, 2017 10:43

NejcZdovc added the PR/needs-QA-attention ☕ label Feb 3, 2017

NejcZdovc assigned bradleyrichter and unassigned NejcZdovc Feb 8, 2017

bsclifton suggested changes Feb 9, 2017

View reviewed changes

NejcZdovc removed the PR/needs-QA-attention ☕ label Feb 9, 2017

NejcZdovc assigned NejcZdovc and unassigned bradleyrichter Feb 10, 2017

NejcZdovc force-pushed the feature/#2110-download branch 2 times, most recently from 8f3c718 to bd77916 Compare February 13, 2017 08:11

bsclifton added the PR/work-in-progress ⚒ label Feb 13, 2017

bbondy modified the milestones: 0.13.6, 0.13.5 Feb 16, 2017

luixxiul added the PR/needs-rebase ۞ label Feb 19, 2017

NejcZdovc force-pushed the feature/#2110-download branch from bd77916 to 34a0eb7 Compare February 27, 2017 17:28

NejcZdovc removed the PR/needs-rebase ۞ label Feb 27, 2017

NejcZdovc force-pushed the feature/#2110-download branch from 34a0eb7 to f4656fb Compare February 27, 2017 20:04

NejcZdovc added PR/needs-QA-attention ☕ and removed PR/work-in-progress ⚒ labels Mar 1, 2017

NejcZdovc force-pushed the feature/#2110-download branch from 95b9397 to f1f4ff1 Compare March 1, 2017 19:43

NejcZdovc force-pushed the feature/#2110-download branch 3 times, most recently from 04878a9 to 577422d Compare March 3, 2017 06:39

bsclifton approved these changes Mar 3, 2017

View reviewed changes

bsclifton suggested changes Mar 3, 2017

View reviewed changes

NejcZdovc force-pushed the feature/#2110-download branch from 58e9152 to a383e96 Compare March 8, 2017 10:51

bsclifton suggested changes Mar 10, 2017

View reviewed changes

NejcZdovc force-pushed the feature/#2110-download branch from a383e96 to c835413 Compare March 10, 2017 20:37

bridiver reviewed Mar 10, 2017

View reviewed changes

NejcZdovc force-pushed the feature/#2110-download branch 3 times, most recently from 0488a54 to d3881eb Compare March 13, 2017 07:31

NejcZdovc force-pushed the feature/#2110-download branch from d3881eb to 88b44fd Compare March 16, 2017 00:07

Custom save as handling

6910800

NejcZdovc requested a review from bsclifton March 16, 2017 00:55

NejcZdovc mentioned this pull request Mar 16, 2017

Fixed style and layout on payment tab #7501

Closed

bsclifton approved these changes Mar 16, 2017

View reviewed changes

bsclifton merged commit 2faf7e9 into brave:master Mar 16, 2017

NejcZdovc removed the PR/needs-QA-attention ☕ label Mar 16, 2017

bradleyrichter mentioned this pull request Mar 17, 2017

Added "Save my downloads here" setting #2110

Closed

cndouglas mentioned this pull request Mar 24, 2017

Tweak download usability problems #1202

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added default download path #7023

Added default download path #7023

NejcZdovc commented Feb 3, 2017 •

edited

Loading

NejcZdovc commented Feb 3, 2017

bsclifton left a comment •

edited

Loading

NejcZdovc commented Feb 9, 2017

bradleyrichter commented Feb 9, 2017

bsclifton commented Feb 16, 2017 •

edited

Loading

NejcZdovc commented Mar 1, 2017

bsclifton left a comment

bsclifton Mar 3, 2017

bsclifton left a comment •

edited

Loading

NejcZdovc commented Mar 8, 2017

bsclifton left a comment •

edited

Loading

NejcZdovc commented Mar 10, 2017

bridiver Mar 10, 2017

NejcZdovc Mar 13, 2017

bridiver commented Mar 10, 2017

NejcZdovc commented Mar 16, 2017

bsclifton commented Mar 16, 2017

bsclifton left a comment

Added default download path #7023

Added default download path #7023

Conversation

NejcZdovc commented Feb 3, 2017 • edited Loading

Auditors

Test Plan

NejcZdovc commented Feb 3, 2017

bsclifton left a comment • edited Loading

Choose a reason for hiding this comment

NejcZdovc commented Feb 9, 2017

bradleyrichter commented Feb 9, 2017

bsclifton commented Feb 16, 2017 • edited Loading

NejcZdovc commented Mar 1, 2017

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton Mar 3, 2017

Choose a reason for hiding this comment

bsclifton left a comment • edited Loading

Choose a reason for hiding this comment

NejcZdovc commented Mar 8, 2017

bsclifton left a comment • edited Loading

Choose a reason for hiding this comment

NejcZdovc commented Mar 10, 2017

bridiver Mar 10, 2017

Choose a reason for hiding this comment

NejcZdovc Mar 13, 2017

Choose a reason for hiding this comment

bridiver commented Mar 10, 2017

NejcZdovc commented Mar 16, 2017

bsclifton commented Mar 16, 2017

bsclifton left a comment

Choose a reason for hiding this comment

NejcZdovc commented Feb 3, 2017 •

edited

Loading

bsclifton left a comment •

edited

Loading

bsclifton commented Feb 16, 2017 •

edited

Loading

bsclifton left a comment •

edited

Loading

bsclifton left a comment •

edited

Loading