MD History: Fix overlapping of sidebar footer in very short windows

MD History: Fix overlapping of sidebar footer in very short windows

The footer was previously absolutely positioned at the bottom of the
sidebar, which caused it to overlap with the sidebar content in very
short windows. This changes the layout of the sidebar so that items
cannot overlap, instead using a scrollbar when necessary.

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

Committed: https://crrev.com/bd29d296277d9115186c6f76e1cebce4eaefc2fc
Cr-Commit-Position: refs/heads/master@{#424686}

[tsergeant, 2016-10-12 00:00:13]

Description was changed from

==========
MD History: Fix overlapping of sidebar footer in very short windows

The footer was previously absolutely positioned at the bottom of the
sidebar, which caused it to overlap with the sidebar content in very
short windows. This changes the layout of the sidebar so that items
cannot overlap, instead using a scrollbar when necessary.

BUG=629737
==========

to

==========
MD History: Fix overlapping of sidebar footer in very short windows

The footer was previously absolutely positioned at the bottom of the
sidebar, which caused it to overlap with the sidebar content in very
short windows. This changes the layout of the sidebar so that items
cannot overlap, instead using a scrollbar when necessary.

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

[tsergeant, 2016-10-12 01:43:19]

Patchset #2 (id:20001) has been deleted

[tsergeant, 2016-10-12 01:44:43]

tsergeant@chromium.org changed reviewers:
+ lshang@chromium.org

[tsergeant, 2016-10-12 01:44:43]

PTAL!

I've posted some screenshots on the bug, let me know if you have any questions
(reviewing CSS can be annoying, so I'm happy to chat in person)

[tsergeant, 2016-10-12 01:44:53]

The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 01:45:00]

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, 2016-10-12 02:36:24]

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

[commit-bot: I haz the power, 2016-10-12 02:36:25]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-10-12 05:15:29]

lgtm with some nits:-)

https://codereview.chromium.org/2412003002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/app.html (right):

https://codereview.chromium.org/2412003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.html:61: box-sizing: border-box;
nit: Is this necessary? This seems to just include the 1px border-bottom line in
the height.

https://codereview.chromium.org/2412003002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/side_bar.html (right):

https://codereview.chromium.org/2412003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:15: flex-direction: column;
nit: There is only one single iron-selector element inside of history-side-bar,
so this is probably not needed. The only difference I can see is the position of
iron-icon besides "Clearing browsing data" when the scroll bar shows.

https://codereview.chromium.org/2412003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:23: height: calc(100% -
var(--toolbar-height));
nit: could this be calc(100% - var(|height of drawer header|))? Even though
drawer-header's height equals var(--toolbar-height), it would be more clear that
the height of rest side bar part is calculated from the side bar header, within
the same element, not from reference of toolbar element, imo.

https://codereview.chromium.org/2412003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:53: padding-top: 5px;
nit: margins between "Tabs from other devices/Clear browsing data" and the
separator line are both 8px, so set padding-top: 8px here probably is better, to
be consistent.

Or you can set it to 10px, as what it is now on tot.

[tsergeant, 2016-10-12 06:06:46]

https://codereview.chromium.org/2412003002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/app.html (right):

https://codereview.chromium.org/2412003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.html:61: box-sizing: border-box;
On 2016/10/12 05:15:28, lshang wrote:
> nit: Is this necessary? This seems to just include the 1px border-bottom line
in
> the height.

This is a very minor thing, but the 1px change here ensures that the height of
the side bar is correct -- otherwise, it extends below the bottom of the window
by 1px.

https://codereview.chromium.org/2412003002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/side_bar.html (right):

https://codereview.chromium.org/2412003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:15: flex-direction: column;
On 2016/10/12 05:15:29, lshang wrote:
> nit: There is only one single iron-selector element inside of
history-side-bar,
> so this is probably not needed. The only difference I can see is the position
of
> iron-icon besides "Clearing browsing data" when the scroll bar shows.

Done.

https://codereview.chromium.org/2412003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:23: height: calc(100% -
var(--toolbar-height));
On 2016/10/12 05:15:29, lshang wrote:
> nit: could this be calc(100% - var(|height of drawer header|))? Even though
> drawer-header's height equals var(--toolbar-height), it would be more clear
that
> the height of rest side bar part is calculated from the side bar header,
within
> the same element, not from reference of toolbar element, imo.

As discussed, I'm leaving this as-is without renaming.

https://codereview.chromium.org/2412003002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/side_bar.html:53: padding-top: 5px;
On 2016/10/12 05:15:29, lshang wrote:
> nit: margins between "Tabs from other devices/Clear browsing data" and the
> separator line are both 8px, so set padding-top: 8px here probably is better,
to
> be consistent.
> 
> Or you can set it to 10px, as what it is now on tot.

I've set it to 8px, which matches Settings

[tsergeant, 2016-10-12 06:07:12]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2016-10-12 06:07:13]

The patchset sent to the CQ was uploaded after l-g-t-m from lshang@chromium.org
Link to the patchset: https://codereview.chromium.org/2412003002/#ps60001
(title: "lshang@ review")

[commit-bot: I haz the power, 2016-10-12 06:07:24]

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, 2016-10-12 07:18:04]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-12 07:19:15]

Description was changed from

==========
MD History: Fix overlapping of sidebar footer in very short windows

The footer was previously absolutely positioned at the bottom of the
sidebar, which caused it to overlap with the sidebar content in very
short windows. This changes the layout of the sidebar so that items
cannot overlap, instead using a scrollbar when necessary.

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

to

==========
MD History: Fix overlapping of sidebar footer in very short windows

The footer was previously absolutely positioned at the bottom of the
sidebar, which caused it to overlap with the sidebar content in very
short windows. This changes the layout of the sidebar so that items
cannot overlap, instead using a scrollbar when necessary.

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

Committed: https://crrev.com/bd29d296277d9115186c6f76e1cebce4eaefc2fc
Cr-Commit-Position: refs/heads/master@{#424686}
==========

[commit-bot: I haz the power, 2016-10-12 07:19:16]

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