Refactor downloads bar with Aphrodite & Add toolbar submenu #11547

luixxiul · 2017-10-15T20:55:18Z

This blocks #10951

Create Toolbar submenu under View (to close Request: Option to hide download bar #10548)

TODO for a follow-up PR: Add Tabs Toolbar and Navigation Bar to hide everything except the menu bar on Windows

Fixes #9320
Closes #10548
Closes #10946
Closes #10948
Addresses #10947

Test Plan:
npm run unittest -- --grep='downloadsToolbarMenuItem'
npm run test -- --grep='Downloads'

Test Plan 2:

Click a 100MB file from http://speedtest.reliableservers.com/
Make sure the Close button is displayed on the download bar
Hover on the file item
Make sure the action buttons are displayed
Click the pause icon
Make sure the transition animation is canceled
Click the trash icon
Make sure the delete confirmation button works

Test Plan 3:

Hide Download Bar from Menu
Download something
Make sure the download bar is hidden

Submitter Checklist:

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).
Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

codecov-io · 2017-10-15T21:18:23Z

Codecov Report

Merging #11547 into master will increase coverage by 0.01%.
The diff coverage is 70.96%.

@@            Coverage Diff             @@
##           master   #11547      +/-   ##
==========================================
+ Coverage   55.95%   55.97%   +0.01%     
==========================================
  Files         278      279       +1     
  Lines       26976    27042      +66     
  Branches     4363     4374      +11     
==========================================
+ Hits        15095    15136      +41     
- Misses      11881    11906      +25

Flag	Coverage Δ
#unittest	`55.97% <70.96%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
js/constants/settings.js	`100% <ø> (ø)`	⬆️
js/contextMenus.js	`18.44% <ø> (ø)`	⬆️
app/common/commonMenu.js	`52.2% <ø> (ø)`	⬆️
js/stores/windowStore.js	`27.42% <ø> (-0.14%)`	⬇️
js/constants/appConfig.js	`100% <ø> (ø)`	⬆️
app/locale.js	`35.77% <ø> (ø)`	⬆️
app/renderer/components/styles/global.js	`100% <ø> (ø)`	⬆️
js/about/preferences.js	`45.98% <ø> (+0.25%)`	⬆️
app/renderer/components/styles/theme.js	`100% <ø> (ø)`	⬆️
app/renderer/components/download/downloadItem.js	`97.08% <100%> (+0.2%)`	⬆️
... and 15 more

bsclifton · 2017-11-13T06:18:43Z

@luixxiul would you be able to rebase this PR + squash it down a bit?

I just reviewed and merged #10882 which affects this same area (download bar). I'd love to review this PR too 😄

luixxiul · 2017-11-13T10:29:44Z

@bsclifton Done 😄

luixxiul · 2017-11-13T10:52:40Z

The test looks good to me 👍

https://travis-ci.org/brave/browser-laptop/jobs/301320701#L5634

   Downloads
    Location and file naming tests
      ✓ check if first download completes (435ms)
      ✓ check if second download completes and is renamed (237ms)
    Item and bar tests
      ✓ check if download bar is shown (246ms)
      ✓ check if you can pause download (361ms)
      ✓ check if you can resume download (262ms)
      ✓ check if you can cancel download (252ms)
      ✓ check if you can re-download (430ms)
      ✓ check if you can remove item from the list (472ms)
      ✓ check if you can delete downloaded item (1341ms)

luixxiul · 2017-11-13T13:29:46Z

less/downloadBar.less

    }
  }
+
+  &.deleteConfirmationVisible:hover {


Obviously we need a nice calculation here ... 😄

bsclifton · 2017-11-21T07:07:21Z

app/renderer/components/download/downloadsBar.js

    props.downloads = downloadUtil.getDownloadItems(state) || Immutable.List()

    return props
  }

  render () {
-    return <div className='downloadsBar'
+    return <div className={cx({
+      [css(styles.downloadsBar, !this.props.showToolbarWhenDownloading && styles.downloadBar_hidden)]: true,


Once possible enhancement- if this setting is true (ex: hide toolbar), it would be better to not show the toolbar in the first place. To do this, you can remove the check here and instead, ONLY set the value in windowReducer.js/menu.js if that value is true. That prevents the component from being created in the first place

That sounds rational. As I am not sure how to implement that, would you mind adding a commit for that? thanks 😄

bsclifton · 2017-11-21T07:09:25Z

Added a commit which changes what the toolbar menu does- instead it will toggle the visibility for the window. I moved the hiding of the downloads toolbar to general, near the other settings:

luixxiul · 2017-11-22T10:38:41Z

@bsclifton Thanks for your commit! I was trying to see how it is like but I got the error and the browser window gets blank:

index-dev.html:1 (WEBUI context) http://localhost:8080/gen/app.entry.js:17861: Uncaught AssertionError: windowId cannot be null{AssertionError: windowId cannot be null
    at validateId (http://localhost:8080/gen/app.entry.js:24825:10)
    at Object.getWindowIndexByWindowId (http://localhost:8080/gen/app.entry.js:24907:16)
    at Object.isVisible (http://localhost:8080/gen/app.entry.js:113238:31)
    at mergeProps (http://localhost:8080/gen/app.entry.js:97267:51)
    at buildPropsImpl (http://localhost:8080/gen/app.entry.js:15771:10)
    at ReduxComponent.buildProps (http://localhost:8080/gen/app.entry.js:15843:12)
    at new ReduxComponent (http://localhost:8080/gen/app.entry.js:15801:23)
    at http://localhost:8080/gen/app.entry.js:81899:18
    at measureLifeCyclePerf (http://localhost:8080/gen/app.entry.js:81680:12)
    at ReactCompositeComponentWrapper._constructComponentWithoutOwner (http://localhost:8080/gen/app.entry.js:81898:16)}

Could I know how to fix that?

cezaraugusto · 2017-12-19T15:12:10Z

cc @luixxiul @bsclifton what's the status of this? there's an error report but some follow-up commits after not sure if this is ready

bsclifton · 2017-12-28T06:31:45Z

@cezaraugusto we need a rebase- I was supposed to re-review after @luixxiul 's latest comment. @luixxiul can you rebase again please 😄

Fixes #9320 Closes #10946 Closes #10948 Addresses #10947 Also: - Move color properties to theme.js Test Plan: 1. `npm run unittest -- --grep='downloadsToolbarMenuItem'` 2. `npm run test -- --grep='Downloads'` Test Plan 2: 1. Click a `100MB` file from `http://speedtest.reliableservers.com/` 2. Make sure the `Close` button is displayed on the download bar 3. Hover on the file item 4. Make sure the action buttons are displayed 5. Click the pause icon 6. Make sure the transition animation is canceled 7. Click the trash icon 8. Make sure the delete confirmation button works

Close #10548 Auditors: Test Plan: 1. Hide `Download Bar` from `Menu` 2. Download something 3. Make sure the download bar is hidden

@luixxiul

- toolbar state removed from window state, moved to app state - menu code updated to properly show checked/unchecked status for toolbar - managing extra state for downloads will be much easier now (ex: if we wanted to track downloads on a per-window basis) Renamed SHOW_TOOLBAR_DOWNLOADS setting as SHOW_TOOLBAR_WHEN_DOWNLOADING - setting was renamed - appconfig value is renamed - setting was moved into general, under downloads area Auditors: @luixxiul

Rename identifiers inside menu.properties

luixxiul · 2018-01-28T19:22:29Z

For now I'm going to close this to work on later.

luixxiul added feature/download refactoring/aphrodite labels Oct 15, 2017

luixxiul self-assigned this Oct 15, 2017

luixxiul requested a review from cezaraugusto October 15, 2017 20:55

luixxiul mentioned this pull request Oct 15, 2017

Replace Button with BrowserButton, set common font properties for BrowserButton, etc. #10951

Closed

8 tasks

luixxiul changed the title ~~Refactor downloads bar~~ Refactor downloads bar with Aphrodite Oct 15, 2017

srirambv mentioned this pull request Oct 15, 2017

Pause download doesn't immediately stop the download indicator #9320

Closed

This was referenced Oct 15, 2017

Add browserButton_smallItem #10946

Closed

Refactor downloadsBar.js with Aphrodite #10948

Closed

Refactor downloadItem.js with Aphrodite #10947

Closed

brave deleted a comment from codecov-io Oct 15, 2017

eljuno mentioned this pull request Oct 16, 2017

Request: Option to hide download bar #10548

Closed

luixxiul added this to the 0.22.x (Nightly Channel) milestone Oct 16, 2017

This was referenced Oct 17, 2017

UI elements and their arrangement should be customizable #6928

Closed

URL and tab bar do not hide in full screen mode #11463

Closed

luixxiul changed the title ~~Refactor downloads bar with Aphrodite~~ Refactor downloads bar with Aphrodite & Add toolbar submenu Oct 17, 2017

luixxiul requested a review from petemill October 17, 2017 18:00

bbondy modified the milestones: 0.22.x (Nightly Channel), Backlog Oct 25, 2017

luixxiul added PR/pending-review QA/test-plan-specified labels Oct 30, 2017

luixxiul added PR/needs-rebase ۞ and removed PR/needs-rebase ۞ labels Nov 7, 2017

bsclifton self-requested a review November 13, 2017 06:16

bsclifton added the PR/needs-rebase ۞ label Nov 13, 2017

luixxiul removed the PR/needs-rebase ۞ label Nov 13, 2017

luixxiul removed the request for review from petemill November 13, 2017 10:29

luixxiul removed the request for review from cezaraugusto November 13, 2017 10:30

luixxiul commented Nov 13, 2017

View reviewed changes

less/downloadBar.less

}

}

&.deleteConfirmationVisible:hover {

Copy link

Contributor Author

luixxiul Nov 13, 2017

Choose a reason for hiding this comment

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

Obviously we need a nice calculation here ... 😄

luixxiul added the PR/needs-rebase ۞ label Nov 20, 2017

bsclifton added PR/needs-rebase ۞ and removed PR/needs-rebase ۞ labels Nov 20, 2017

bsclifton reviewed Nov 21, 2017

View reviewed changes

luixxiul removed the PR/needs-rebase ۞ label Nov 22, 2017

cezaraugusto added the needs-info Another team member needs information from the PR/issue opener. label Dec 19, 2017

bsclifton modified the milestones: Triage Backlog, 0.22.x (Nightly Channel) Dec 28, 2017

Suguru Hirahara and others added 4 commits January 7, 2018 11:32

Add 'View -> Toolbar' menu

72a6f2a

Close #10548 Auditors: Test Plan: 1. Hide `Download Bar` from `Menu` 2. Download something 3. Make sure the download bar is hidden

Integrate toolbars menu commented out

e836688

Rename identifiers inside menu.properties

luixxiul closed this Jan 28, 2018

luixxiul added PR/work-in-progress ⚒ and removed needs-info Another team member needs information from the PR/issue opener. labels Jan 28, 2018

luixxiul removed this from the 0.22.x (Nightly Channel) milestone Jan 28, 2018

bsclifton removed the PR/pending-review label Feb 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor downloads bar with Aphrodite & Add toolbar submenu #11547

Refactor downloads bar with Aphrodite & Add toolbar submenu #11547

luixxiul commented Oct 15, 2017 •

edited

Loading

codecov-io commented Oct 15, 2017 •

edited

Loading

bsclifton commented Nov 13, 2017

luixxiul commented Nov 13, 2017

luixxiul commented Nov 13, 2017

luixxiul Nov 13, 2017

bsclifton Nov 21, 2017

luixxiul Nov 22, 2017

bsclifton commented Nov 21, 2017 •

edited

Loading

luixxiul commented Nov 22, 2017 •

edited

Loading

cezaraugusto commented Dec 19, 2017

bsclifton commented Dec 28, 2017

luixxiul commented Jan 28, 2018

Refactor downloads bar with Aphrodite & Add toolbar submenu #11547

Refactor downloads bar with Aphrodite & Add toolbar submenu #11547

Conversation

luixxiul commented Oct 15, 2017 • edited Loading

codecov-io commented Oct 15, 2017 • edited Loading

Codecov Report

bsclifton commented Nov 13, 2017

luixxiul commented Nov 13, 2017

luixxiul commented Nov 13, 2017

luixxiul Nov 13, 2017

Choose a reason for hiding this comment

bsclifton Nov 21, 2017

Choose a reason for hiding this comment

luixxiul Nov 22, 2017

Choose a reason for hiding this comment

bsclifton commented Nov 21, 2017 • edited Loading

luixxiul commented Nov 22, 2017 • edited Loading

cezaraugusto commented Dec 19, 2017

bsclifton commented Dec 28, 2017

luixxiul commented Jan 28, 2018

luixxiul commented Oct 15, 2017 •

edited

Loading

codecov-io commented Oct 15, 2017 •

edited

Loading

bsclifton commented Nov 21, 2017 •

edited

Loading

luixxiul commented Nov 22, 2017 •

edited

Loading