remoting/webapp/html/title_bar.html - Issue 265393005: Implement apps v2 custom window frame.

Side by Side Diff: remoting/webapp/html/title_bar.html

Issue 265393005: Implement apps v2 custom window frame. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: Removed file added accidentally. Created 6 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch | Annotate | Revision Log

OLD	NEW
(Empty)
	1 <!--

	2 Copyright (c) 2014 The Chromium Authors. All rights reserved.

	3 Use of this source code is governed by a BSD-style license that can be

	4 found in the LICENSE file.

	5 -->

	6 <div id="title-bar" class="title-bar apps-v2-only">

	7 <span id="window-title" class="window-title"> </span>
	Jamie 2014/05/05 22:37:15 I don't like the use of   here, but the size I don't like the use of   here, but the size specifications for the span are not respected without it. If you can think of a better solution, please let me know. Sergey Ulanov 2014/05/06 07:58:14 nit: do you need both class and id here? nit: do you need both class and id here? Sergey Ulanov 2014/05/06 07:58:14 Does it not work if you put a space instead of &nb Show quoted text On 2014/05/05 22:37:15, Jamie wrote: > I don't like the use of   here, but the size specifications for the span > are not respected without it. If you can think of a better solution, please let > me know. Does it not work if you put a space instead of   here? Jamie 2014/05/06 21:11:08 I don't think so. Even if it did, I would be nervo Show quoted text On 2014/05/06 07:58:14, Sergey Ulanov wrote: > On 2014/05/05 22:37:15, Jamie wrote: > > I don't like the use of   here, but the size specifications for the span > > are not respected without it. If you can think of a better solution, please > let > > me know. > > Does it not work if you put a space instead of   here? I don't think so. Even if it did, I would be nervous about it being removed by a future edit that thought it was unnecessary white-space. Jamie 2014/05/06 21:11:08 I've eliminated all ids except the top-level one i Show quoted text On 2014/05/06 07:58:14, Sergey Ulanov wrote: > nit: do you need both class and id here? I've eliminated all ids except the top-level one in favour of a pattern whereby the root node is passed to the WindowFrame class, which uses querySelector to find the child nodes.
	8 <span class="window-controls-hover-target" id="window-controls">
	Sergey Ulanov 2014/05/06 07:58:14 Maybe use divs instead of spans and explicitly set Maybe use divs instead of spans and explicitly set display to inline-block? Otherwise presentation is not separated from the content. Also it's confusing when divs are mixed with spans. Jamie 2014/05/06 21:11:08 I've used spans for content that is laid out horiz Show quoted text On 2014/05/06 07:58:14, Sergey Ulanov wrote: > Maybe use divs instead of spans and explicitly set display to inline-block? > Otherwise presentation is not separated from the content. Also it's confusing > when divs are mixed with spans. I've used spans for content that is laid out horizontally and divs for content that is laid out vertically. I think that more clearly describes the layout than using divs everywhere. LMK if you disagree; the CSS overrides the defaults sufficiently that I don't think it makes any difference and I don't have a strong preference either way. It turns out there was an extra unnecessary div, which I've removed, but I've left everything else unchanged.
	9 <span class="window-controls">
	Sergey Ulanov 2014/05/06 07:58:14 you have both id=window-controls and class=window- you have both id=window-controls and class=window-controls. This might be confusing and I'm not sure if id=window-controls is useful at all. Jamie 2014/05/06 21:11:08 See above. Show quoted text On 2014/05/06 07:58:14, Sergey Ulanov wrote: > you have both id=window-controls and class=window-controls. This might be > confusing and I'm not sure if id=window-controls is useful at all. See above.
	10 <div>
	kelvinp 2014/05/06 04:42:15 <span>'s are inline elements and <div>'s are block <span>'s are inline elements and <div>'s are block elements. Is it a particular reason why the div is sandwiched between the spans here? Can we replace the div with a span? Jamie 2014/05/06 21:11:08 Done. Show quoted text On 2014/05/06 04:42:15, kelvinp wrote: > <span>'s are inline elements and <div>'s are block elements. Is it a particular > reason why the div is sandwiched between the spans here? Can we replace the div > with a span? Done.
	11 <span id="window-disconnect"

	12 i18n-title="DISCONNECT_MYSELF_BUTTON"

	13 class="window-control">

	14 <img src="icon_disconnect.webp">

	15 </span>

	16 <span id="window-minimize"
	kelvinp 2014/05/06 04:42:15 Do we care about RTL languages? Do we need to sho Do we care about RTL languages? Do we need to show the buttons on the top-right corner on those locales? Jamie 2014/05/06 21:11:08 Good catch! We do care, and I think the correct be Show quoted text On 2014/05/06 04:42:15, kelvinp wrote: > Do we care about RTL languages? Do we need to show the buttons on the top-right > corner on those locales? Good catch! We do care, and I think the correct behaviour is to lay out the controls left-aligned. I'm not certain though, and it might be a bit fiddly to get it working, so I would prefer to follow up with a CL to address just that after confirming that it's the right behaviour.
	17 i18n-title="MINIMIZE_WINDOW"

	18 class="window-control">

	19 <img src="icon_minimize.webp">

	20 </span>

	21 <span id="window-maximize-restore"

	22 i18n-title="MAXIMIZE_WINDOW"

	23 class="window-control">

	24 <img src="icon_maximize_restore.webp">

	25 </span>

	26 <span id="window-close"

	27 i18n-title="CLOSE_WINDOW"

	28 class="window-control">

	29 <img src="icon_close.webp">

	30 </span>

	31 </div>

	32 <div id="window-controls-stub"> </div>

	33 </span>

	34 </span>

	35 </div>

OLD	NEW

« remoting/resources/remoting_strings.grd ('K') | « remoting/webapp/html/template_main.html ('k') | remoting/webapp/html/toolbar.html » ('j') | remoting/webapp/title_bar.css » ('J')