[MD Bookmarks] Add lines between items and no search results message.

[MD Bookmarks] Add lines between items and no search results message.


BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2639893006
Cr-Commit-Position: refs/heads/master@{#445330}
Committed: https://chromium.googlesource.com/chromium/src/+/f37249e1ef4c5c6bdf2588ebaec7ffa748be33c3

[angelayang, 2017-01-19 04:42:24]

Description was changed from

==========
[MD Bookmarks] Add lines between items and no search results message.


BUG=658980
==========

to

==========
[MD Bookmarks] Add lines between items and no search results message.


BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[angelayang, 2017-01-19 04:43:13]

Description was changed from

==========
[MD Bookmarks] Add lines between items and no search results message.


BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Add lines between items and no search results message.


BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[angelayang, 2017-01-19 04:43:13]

angelayang@google.com changed reviewers:
+ jiaxi@google.com

[jiaxi, 2017-01-19 04:50:57]

https://codereview.chromium.org/2639893006/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/item.html (right):

https://codereview.chromium.org/2639893006/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/item.html:8: 
Delete extra line here.

https://codereview.chromium.org/2639893006/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.chromium.org/2639893006/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/list.html:37: 
Extra line here.

https://codereview.chromium.org/2639893006/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2639893006/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/list.js:101: var lastItem =
this.displayedList[this.displayedList.length - 1];
item.index == this.displayedList.length - 1

[angelayang, 2017-01-19 05:00:22]

angelayang@google.com changed reviewers:
+ calamity@chromium.org, tsergeant@chromium.org

[angelayang, 2017-01-19 05:00:23]

Hi, I've made a patch for the separator lines and added in a message for empty
results.

take a look thanks

https://codereview.chromium.org/2639893006/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/item.html (right):

https://codereview.chromium.org/2639893006/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/item.html:8: 
On 2017/01/19 04:50:57, jiaxi wrote:
> Delete extra line here.

Done.

https://codereview.chromium.org/2639893006/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.chromium.org/2639893006/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/list.html:37: 
On 2017/01/19 04:50:57, jiaxi wrote:
> Extra line here.

Done.

https://codereview.chromium.org/2639893006/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2639893006/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/list.js:101: var lastItem =
this.displayedList[this.displayedList.length - 1];
On 2017/01/19 04:50:57, jiaxi wrote:
> item.index == this.displayedList.length - 1

Done.

[jiaxi, 2017-01-19 05:03:13]

lgtm

[jiaxi, 2017-01-19 05:03:15]

lgtm

[tsergeant, 2017-01-19 05:25:41]

https://codereview.chromium.org/2639893006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/item.html (right):

https://codereview.chromium.org/2639893006/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/item.html:19: :host(:not([is-last-item]))
{
I -think- you can replace is-last-item with a CSS pseudoclass:

:host(:not(:last-child))

which should match every <bookmarks-item> other than the last.

https://codereview.chromium.org/2639893006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.chromium.org/2639893006/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.html:78:
{{emptyListMessage_(displayedList.length)}}
[[]] instead of {{}}

Also, you should add searchTerm to the arguments of this function, since you
want to re-evaluate it when the search changes.

[angelayang, 2017-01-20 02:33:02]

patch up :) take a look? thanks

https://codereview.chromium.org/2639893006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/item.html (right):

https://codereview.chromium.org/2639893006/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/item.html:19: :host(:not([is-last-item]))
{
On 2017/01/19 05:25:41, tsergeant wrote:
> I -think- you can replace is-last-item with a CSS pseudoclass:
> 
> :host(:not(:last-child))
> 
> which should match every <bookmarks-item> other than the last. 

Thankyou, very useful!

https://codereview.chromium.org/2639893006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.chromium.org/2639893006/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.html:78:
{{emptyListMessage_(displayedList.length)}}
On 2017/01/19 05:25:41, tsergeant wrote:
> [[]] instead of {{}}
> 
> Also, you should add searchTerm to the arguments of this function, since you
> want to re-evaluate it when the search changes.

Done.

[tsergeant, 2017-01-20 02:48:27]

https://codereview.chromium.org/2639893006/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.chromium.org/2639893006/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.html:77:
[[emptyListMessage_(displayedList.length, searchTerm)]]
In fact, emptyListMessage_(searchTerm) is fine since the message only changes
when the search changes.

https://codereview.chromium.org/2639893006/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2639893006/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:15: searchTerm: {
Does it work if you use the short form:

searchTerm: String ?

(we try to avoid setting default values unless they're necessary, since they
cause all the observers to fire)

[angelayang, 2017-01-23 00:58:15]

Hi I changed it so that the message updates according to the searchTerm.

[tsergeant, 2017-01-23 02:03:41]

https://codereview.chromium.org/2639893006/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.chromium.org/2639893006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.html:77:
[[emptyListMessage_(displayedList)]]
This updates based on the displayedList, not based on the searchTerm?

[angelayang, 2017-01-23 02:42:30]

On 2017/01/23 02:03:41, tsergeant wrote:
>
https://codereview.chromium.org/2639893006/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/md_bookmarks/list.html (right):
> 
>
https://codereview.chromium.org/2639893006/diff/60001/chrome/browser/resource...
> chrome/browser/resources/md_bookmarks/list.html:77:
> [[emptyListMessage_(displayedList)]]
> This updates based on the displayedList, not based on the searchTerm?

Sorry my mistake. I'll fix this up.

[angelayang, 2017-01-23 02:49:07]

Patchset #4 (id:60001) has been deleted

[tsergeant, 2017-01-23 04:52:54]

https://codereview.chromium.org/2639893006/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/item.html (right):

https://codereview.chromium.org/2639893006/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/item.html:20: border-bottom: 1px solid
rgba(0, 0, 0, 0.06);
One last little thing (sorry!)

Change this to

border-bottom: var(--cr-separator-line)

Then, in shared_vars.html, import

chrome://resources/cr_elements/shared_vars_css.html

This ensures that bookmarks uses the same border as settings, even if settings
decides to change it.

[angelayang, 2017-01-23 05:58:39]

All cleaned up, there is still the flashing of the message on load, but Tim and
I discussed to have that fixed in separate cl.
thanks

https://codereview.chromium.org/2639893006/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/item.html (right):

https://codereview.chromium.org/2639893006/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/item.html:20: border-bottom: 1px solid
rgba(0, 0, 0, 0.06);
On 2017/01/23 04:52:54, tsergeant wrote:
> One last little thing (sorry!)
> 
> Change this to
> 
> border-bottom: var(--cr-separator-line)
> 
> Then, in shared_vars.html, import
> 
> chrome://resources/cr_elements/shared_vars_css.html
> 
> This ensures that bookmarks uses the same border as settings, even if settings
> decides to change it.

Oh of course, thats important

[calamity, 2017-01-23 06:03:34]

lgtm

[tsergeant, 2017-01-23 06:07:43]

lgtm

[angelayang, 2017-01-23 06:08:18]

The CQ bit was checked by angelayang@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 06:08:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 06:55:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 06:55:01]

Dry run: This issue passed the CQ dry run.

[angelayang, 2017-01-23 06:55:31]

The CQ bit was checked by angelayang@google.com

[angelayang, 2017-01-23 06:55:31]

The patchset sent to the CQ was uploaded after l-g-t-m from jiaxi@google.com
Link to the patchset: https://codereview.chromium.org/2639893006/#ps120001
(title: "Fix css on separator line")

[commit-bot: I haz the power, 2017-01-23 06:55:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 06:59:51]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485154531249690,
"parent_rev": "adddab40528d1fb6dd94c0e4d2bc794ffccf194a", "commit_rev":
"f37249e1ef4c5c6bdf2588ebaec7ffa748be33c3"}

[commit-bot: I haz the power, 2017-01-23 07:00:52]

Description was changed from

==========
[MD Bookmarks] Add lines between items and no search results message.


BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Add lines between items and no search results message.


BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2639893006
Cr-Commit-Position: refs/heads/master@{#445330}
Committed:
https://chromium.googlesource.com/chromium/src/+/f37249e1ef4c5c6bdf2588ebaec7...
==========

[commit-bot: I haz the power, 2017-01-23 07:00:54]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/f37249e1ef4c5c6bdf2588ebaec7...