Issue 900743002: Move preview panel's components to toolbar and footer.

Issue 900743002: Move preview panel's components to toolbar and footer. (Closed)

Created:
5 years, 10 months ago by fukino

Modified:
5 years, 10 months ago

Reviewers:
hirono, Evan Stade

CC:
chromium-reviews, nkostylev+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, oshima+watch_chromium.org, vitalyp+closure_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, dbeam+watch-closure_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Move preview panel's components to toolbar and footer. - Combobutton is re-styled to fit in the toolbar. - Task/share/delete buttons are moved to the toolbar. - OK/Cancel/NewFolder buttons, which are shown in open-dialog, are moved to the footer. - Preview panel is removed. BUG=402344, 452802 Committed: https://crrev.com/f6e654986dbda503d573f7551973264219e8fb1f Cr-Commit-Position: refs/heads/master@{#314814}

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : Removed a test which checks thumbnail on the preview panel. #

Total comments: 26

Patch Set 4 : Respond to review comments and rebase. #

Patch Set 5 : Remove dragselection events. #

Patch Set 6 : Add a TODO comment about customized CommandButton. #

Created: 5 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+136 lines, -1171 lines)			Patch
M	chrome/browser/chromeos/file_manager/file_manager_browsertest.cc	View	1 2	1 chunk	+0 lines, -12 lines	0 comments	Download
M	chrome/browser/chromeos/file_manager/file_manager_jstest.cc	View	1 2 3	1 chunk	+0 lines, -10 lines	0 comments	Download
M	ui/file_manager/file_manager/foreground/css/combobutton.css	View		1 chunk	+20 lines, -12 lines	0 comments	Download
M	ui/file_manager/file_manager/foreground/css/file_manager.css	View		5 chunks	+33 lines, -31 lines	0 comments	Download
M	ui/file_manager/file_manager/foreground/js/compiled_resources.gyp	View	1 2 3	2 chunks	+0 lines, -2 lines	0 comments	Download
M	ui/file_manager/file_manager/foreground/js/file_manager.js	View	1 2 3	1 chunk	+0 lines, -8 lines	0 comments	Download
M	ui/file_manager/file_manager/foreground/js/file_selection.js	View	1 2 3	7 chunks	+21 lines, -114 lines	0 comments	Download
M	ui/file_manager/file_manager/foreground/js/main_scripts.js	View		2 chunks	+0 lines, -2 lines	0 comments	Download
D	ui/file_manager/file_manager/foreground/js/preview_panel_model.js	View		1 chunk	+0 lines, -113 lines	0 comments	Download
D	ui/file_manager/file_manager/foreground/js/preview_panel_model_unittest.html	View		1 chunk	+0 lines, -17 lines	0 comments	Download
D	ui/file_manager/file_manager/foreground/js/preview_panel_model_unittest.js	View		1 chunk	+0 lines, -73 lines	0 comments	Download
M	ui/file_manager/file_manager/foreground/js/ui/combobutton.js	View		1 chunk	+0 lines, -16 lines	0 comments	Download
M	ui/file_manager/file_manager/foreground/js/ui/commandbutton.js	View	1 2 3 4 5	2 chunks	+11 lines, -2 lines	0 comments	Download
M	ui/file_manager/file_manager/foreground/js/ui/dialog_footer.js	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	ui/file_manager/file_manager/foreground/js/ui/drag_selector.js	View	1 2 3 4	2 chunks	+0 lines, -2 lines	0 comments	Download
M	ui/file_manager/file_manager/foreground/js/ui/file_manager_ui.js	View	1 2 3	5 chunks	+9 lines, -68 lines	0 comments	Download
D	ui/file_manager/file_manager/foreground/js/ui/preview_panel.js	View		1 chunk	+0 lines, -543 lines	0 comments	Download
D	ui/file_manager/file_manager/foreground/js/ui/preview_panel_unittest.html	View		1 chunk	+0 lines, -12 lines	0 comments	Download
D	ui/file_manager/file_manager/foreground/js/ui/preview_panel_unittest.js	View		1 chunk	+0 lines, -48 lines	0 comments	Download
M	ui/file_manager/file_manager/foreground/js/ui/search_box.js	View	1 2 3	1 chunk	+9 lines, -0 lines	0 comments	Download
M	ui/file_manager/file_manager/main.html	View		6 chunks	+29 lines, -44 lines	0 comments	Download
D	ui/file_manager/integration_tests/file_manager/thumbnails.js	View	1 2	1 chunk	+0 lines, -39 lines	0 comments	Download
M	ui/file_manager/integration_tests/file_manager_test_manifest.json	View	1 2	1 chunk	+0 lines, -1 line	0 comments	Download
M	ui/webui/resources/polymer_resources.grdp	View	1 2 3	1 chunk	+3 lines, -0 lines	0 comments	Download

Messages

Total messages: 17 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

fukino

estade@: PTAL ui/webui/resources/polymer_resources.grdp. hirono@: PTAL others. Thanks!

5 years, 10 months ago (2015-02-04 01:46:44 UTC) #2

hirono

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_manager/foreground/css/combobutton.css File ui/file_manager/file_manager/foreground/css/combobutton.css (right): https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_manager/foreground/css/combobutton.css#newcode13 ui/file_manager/file_manager/foreground/css/combobutton.css:13: margin: 0 0.29em; Why is this em, not px? ...

5 years, 10 months ago (2015-02-04 02:29:51 UTC) #3

hirono

5 years, 10 months ago (2015-02-04 02:29:52 UTC) #4

fukino

Thank you! https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_manager/foreground/css/combobutton.css File ui/file_manager/file_manager/foreground/css/combobutton.css (right): https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_manager/foreground/css/combobutton.css#newcode13 ui/file_manager/file_manager/foreground/css/combobutton.css:13: margin: 0 0.29em; On 2015/02/04 02:29:51, hirono ...

5 years, 10 months ago (2015-02-04 03:27:26 UTC) #5

hirono

5 years, 10 months ago (2015-02-04 04:03:11 UTC) #6

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/css/combobutton.css (right):

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/css/combobutton.css:13: margin: 0
0.29em;
On 2015/02/04 03:27:26, fukino wrote:
> On 2015/02/04 02:29:51, hirono wrote:
> > Why is this em, not px?
> 
> Basically I copied these values from paper-button.
> I'd like to keep it as is until we completely switch to paper-button.

Got it. Thanks!

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/file_selection.js (left):

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_selection.js:352:
list.scrollIndexIntoView(list.selectionModel.selectedIndex);
On 2015/02/04 03:27:26, fukino wrote:
> On 2015/02/04 02:29:51, hirono wrote:
> > Is it OK drop these lines?
> 
> I think this line was needed to handle the following scenario.
> 1. No items are selected, so preview panel is not present.
> 2. Select the bottom item. This makes the preview panel shown and the preview
> panel hides the selected item.
> 
> Without preview panel, I think we can drop these lines.

Ah yes, you are right!

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/file_selection.js (right):

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_selection.js:239: // Update the
label "x files selected." on the header.
On 2015/02/04 03:27:26, fukino wrote:
> On 2015/02/04 02:29:51, hirono wrote:
> > I'm not sure if it could be done easily. But could you consider update these
> > properties in UI class of Header by listing selection change event? or can
we
> > add TODO about this?
> 
> I'm thinking of making a presenter class for the toolbar.
> Can I do it after the spec fix and feature freeze?
> I added a TODO comment.

Thanks!

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_selection.js:256:
this.fileManager_.ui.searchBox.setHidden(selection.totalCount !== 0);
On 2015/02/04 03:27:26, fukino wrote:
> On 2015/02/04 02:29:51, hirono wrote:
> > ditto: Can we do this in SearchBox UI class?
> 
> Added a TODO comment for this, too.

Thanks!

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/commandbutton.js (right):

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/commandbutton.js:102:
this.textContent = label;
On 2015/02/04 03:27:26, fukino wrote:
> On 2015/02/04 02:29:51, hirono wrote:
> > Can the line be executed? Can we remove setLabel method completely?
> 
> Normal buttons with text don't have child element, so the textContent will be
> overwritten by the command's label.
> In Files.app, |this.firstElementChild| will be true only when the button has
an
> icon as a child element (And in this case, we want to keep the icon, so avoid
> overwrite textContent).

My concern is it is unclear who has the responsibility of updating the button's
contents.
If the button has the complex contents including both icon and text, no one
handles label updating.

Rough idea is adding 'icon' attribute to the command button and generates DOM
dynamically in the class. In this case, the class still keep responsibility to
update its contents.

WDYT?

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/file_manager_ui.js (left):

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/file_manager_ui.js:272:
table.list.addEventListener('drag', draggingBound);
On 2015/02/04 03:27:26, fukino wrote:
> On 2015/02/04 02:29:51, hirono wrote:
> > Can we remove these drag related events from list and grid?
> 
> IIUC, these drag-event listeners are made to hide preview panel while
dragging.
> Now that the preview panel is gone, I think we don't need these events
anymore. 

So please remove these events as well as the event handlers.

fukino

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_manager/foreground/js/ui/commandbutton.js File ui/file_manager/file_manager/foreground/js/ui/commandbutton.js (right): https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_manager/foreground/js/ui/commandbutton.js#newcode102 ui/file_manager/file_manager/foreground/js/ui/commandbutton.js:102: this.textContent = label; On 2015/02/04 04:03:11, hirono wrote: > ...

5 years, 10 months ago (2015-02-04 06:22:03 UTC) #7

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/commandbutton.js (right):

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/commandbutton.js:102:
this.textContent = label;
On 2015/02/04 04:03:11, hirono wrote:
> On 2015/02/04 03:27:26, fukino wrote:
> > On 2015/02/04 02:29:51, hirono wrote:
> > > Can the line be executed? Can we remove setLabel method completely?
> > 
> > Normal buttons with text don't have child element, so the textContent will
be
> > overwritten by the command's label.
> > In Files.app, |this.firstElementChild| will be true only when the button has
> an
> > icon as a child element (And in this case, we want to keep the icon, so
avoid
> > overwrite textContent).
> 
> My concern is it is unclear who has the responsibility of updating the
button's
> contents.
> If the button has the complex contents including both icon and text, no one
> handles label updating.
> 
> Rough idea is adding 'icon' attribute to the command button and generates DOM
> dynamically in the class. In this case, the class still keep responsibility to
> update its contents.
> 
> WDYT?

If we consider a customized button content like icon+text, I think only who
customized the button content can update the content appropriately.
So, my approach (#1) was:
- Default button content is command's label. (In this case the content can be
updated automatically.)
- If the button content is customized, the content needs to be updated by the
user (who customized the content).

Alternatively,
#2: If we limit the button content in a few patterns such as "Simple text" or
"Simple icon", CommandButton can handle each cases by checking icon attribute
(as you suggested).
#3: Add pre-defined class to a element in customized button content, which means
its textConent needs to be synced with command.label.

What I am thinking is, go with #1 for now, and switch to #3 when we convert
CommandButton as a polymer element using template binding. WDYT?

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/file_manager_ui.js (left):

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/file_manager_ui.js:272:
table.list.addEventListener('drag', draggingBound);
On 2015/02/04 04:03:11, hirono wrote:
> On 2015/02/04 03:27:26, fukino wrote:
> > On 2015/02/04 02:29:51, hirono wrote:
> > > Can we remove these drag related events from list and grid?
> > 
> > IIUC, these drag-event listeners are made to hide preview panel while
> dragging.
> > Now that the preview panel is gone, I think we don't need these events
> anymore. 
> 
> So please remove these events as well as the event handlers.

Ah, I was misunderstanding.
I removed 'dragselectionstart' and 'dragselectionend'. ('drag' and 'dragend' are
standard events.)

hirono

Thanks, lgtm! https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_manager/foreground/js/ui/commandbutton.js File ui/file_manager/file_manager/foreground/js/ui/commandbutton.js (right): https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_manager/foreground/js/ui/commandbutton.js#newcode102 ui/file_manager/file_manager/foreground/js/ui/commandbutton.js:102: this.textContent = label; On 2015/02/04 06:22:02, fukino ...

5 years, 10 months ago (2015-02-04 07:27:22 UTC) #8

Thanks, lgtm!

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/commandbutton.js (right):

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/commandbutton.js:102:
this.textContent = label;
On 2015/02/04 06:22:02, fukino wrote:
> On 2015/02/04 04:03:11, hirono wrote:
> > On 2015/02/04 03:27:26, fukino wrote:
> > > On 2015/02/04 02:29:51, hirono wrote:
> > > > Can the line be executed? Can we remove setLabel method completely?
> > > 
> > > Normal buttons with text don't have child element, so the textContent will
> be
> > > overwritten by the command's label.
> > > In Files.app, |this.firstElementChild| will be true only when the button
has
> > an
> > > icon as a child element (And in this case, we want to keep the icon, so
> avoid
> > > overwrite textContent).
> > 
> > My concern is it is unclear who has the responsibility of updating the
> button's
> > contents.
> > If the button has the complex contents including both icon and text, no one
> > handles label updating.
> > 
> > Rough idea is adding 'icon' attribute to the command button and generates
DOM
> > dynamically in the class. In this case, the class still keep responsibility
to
> > update its contents.
> > 
> > WDYT?
> 
> If we consider a customized button content like icon+text, I think only who
> customized the button content can update the content appropriately.
> So, my approach (#1) was:
> - Default button content is command's label. (In this case the content can be
> updated automatically.)
> - If the button content is customized, the content needs to be updated by the
> user (who customized the content).
> 
> Alternatively,
> #2: If we limit the button content in a few patterns such as "Simple text" or
> "Simple icon", CommandButton can handle each cases by checking icon attribute
> (as you suggested).
> #3: Add pre-defined class to a element in customized button content, which
means
> its textConent needs to be synced with command.label.
> 
> What I am thinking is, go with #1 for now, and switch to #3 when we convert
> CommandButton as a polymer element using template binding. WDYT?

SGTM. Please add TODO comment about this plan.

fukino

5 years, 10 months ago (2015-02-04 22:54:37 UTC) #10

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/commandbutton.js (right):

https://codereview.chromium.org/900743002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/commandbutton.js:102:
this.textContent = label;
On 2015/02/04 07:27:21, hirono wrote:
> On 2015/02/04 06:22:02, fukino wrote:
> > On 2015/02/04 04:03:11, hirono wrote:
> > > On 2015/02/04 03:27:26, fukino wrote:
> > > > On 2015/02/04 02:29:51, hirono wrote:
> > > > > Can the line be executed? Can we remove setLabel method completely?
> > > > 
> > > > Normal buttons with text don't have child element, so the textContent
will
> > be
> > > > overwritten by the command's label.
> > > > In Files.app, |this.firstElementChild| will be true only when the button
> has
> > > an
> > > > icon as a child element (And in this case, we want to keep the icon, so
> > avoid
> > > > overwrite textContent).
> > > 
> > > My concern is it is unclear who has the responsibility of updating the
> > button's
> > > contents.
> > > If the button has the complex contents including both icon and text, no
one
> > > handles label updating.
> > > 
> > > Rough idea is adding 'icon' attribute to the command button and generates
> DOM
> > > dynamically in the class. In this case, the class still keep
responsibility
> to
> > > update its contents.
> > > 
> > > WDYT?
> > 
> > If we consider a customized button content like icon+text, I think only who
> > customized the button content can update the content appropriately.
> > So, my approach (#1) was:
> > - Default button content is command's label. (In this case the content can
be
> > updated automatically.)
> > - If the button content is customized, the content needs to be updated by
the
> > user (who customized the content).
> > 
> > Alternatively,
> > #2: If we limit the button content in a few patterns such as "Simple text"
or
> > "Simple icon", CommandButton can handle each cases by checking icon
attribute
> > (as you suggested).
> > #3: Add pre-defined class to a element in customized button content, which
> means
> > its textConent needs to be synced with command.label.
> > 
> > What I am thinking is, go with #1 for now, and switch to #3 when we convert
> > CommandButton as a polymer element using template binding. WDYT?
> 
> SGTM. Please add TODO comment about this plan.

Done.

Evan Stade

lgtm feel free to tbr me for mechanical changes like this

5 years, 10 months ago (2015-02-05 03:14:13 UTC) #12

fukino

On 2015/02/05 03:14:13, Evan Stade wrote: > lgtm > > feel free to tbr me ...

5 years, 10 months ago (2015-02-05 15:37:19 UTC) #13

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/900743002/100001

5 years, 10 months ago (2015-02-05 15:38:18 UTC) #15

commit-bot: I haz the power

5 years, 10 months ago (2015-02-05 15:43:15 UTC) #17

Message was sent while issue was closed.

Patchset 6 (id:??) landed as
https://crrev.com/f6e654986dbda503d573f7551973264219e8fb1f
Cr-Commit-Position: refs/heads/master@{#314814}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages