MD Settings: Polish languages pages.

MD Settings: Polish languages pages.

Fix layout and formatting issues and address updated mocks
Use real values for secondary labels
Link to language detail page from spellcheck items
Minor performance enhancements

BUG=512479
Committed: https://crrev.com/4d04ee92fa89e743089f6547a46015a1bb27b514
Cr-Commit-Position: refs/heads/master@{#386169}

[michaelpg, 2016-04-04 18:23:40]

Description was changed from

==========
MD Settings: Polish languages pages.

BUG=512479
==========

to

==========
MD Settings: Polish languages pages.

Not ready for review

BUG=512479
==========

[michaelpg, 2016-04-06 00:07:44]

Patchset #2 (id:20001) has been deleted

[michaelpg, 2016-04-06 00:07:46]

Patchset #1 (id:1) has been deleted

[michaelpg, 2016-04-06 00:11:26]

Description was changed from

==========
MD Settings: Polish languages pages.

Not ready for review

BUG=512479
==========

to

==========
MD Settings: Polish languages pages.

Fix layout and formatting issues and address updated mocks
Use real values for secondary labels
Link to language detail page from spellcheck items
Minor performance enhancements
==========

[michaelpg, 2016-04-06 00:11:26]

michaelpg@chromium.org changed reviewers:
+ dschuyler@chromium.org

[michaelpg, 2016-04-06 02:36:13]

PTAL. Screenshots:

https://screenshot.googleplex.com/FNmX9anUR2x
https://screenshot.googleplex.com/wP2fkjHPRP2
https://screenshot.googleplex.com/fO3Q4Bz5to0
https://screenshot.googleplex.com/RzuRQBKbOKU

[dschuyler, 2016-04-06 21:45:50]

https://codereview.chromium.org/1856873002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/1856873002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:26:
font-weight: 500;
Would it make sense to have this as 
list-item[selected] {
  font-weight: 500;
}
in settings_share_css? (or is there any reason
not to have it there)?

https://codereview.chromium.org/1856873002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/manage_languages_page.html
(right):

https://codereview.chromium.org/1856873002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/manage_languages_page.html:26:
<div  class="start">
Is that extra space intentional?

https://codereview.chromium.org/1856873002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/manage_languages_page.html:28:
<div id="languages-list" class="list-frame vertical-list">
The style doc (below) suggests using camel
case id names. 

https://docs.google.com/document/d/1G0t519y_cSW4etXVwruR145abZH0N5-9R7JyejzVo...

https://codereview.chromium.org/1856873002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/manage_languages_page.html:47:
disabled="[[!canRemoveLanguage_(item.code, prefs.intl.app_locale.value)]]"
I think we are now able to wrap these lines,
can this line be wrapped?

[michaelpg, 2016-04-07 14:55:47]

PTAL, and optionally weigh in on indent question at
https://docs.google.com/document/d/1G0t519y_cSW4etXVwruR145abZH0N5-9R7JyejzVo...

https://codereview.chromium.org/1856873002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/1856873002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.html:26:
font-weight: 500;
On 2016/04/06 21:45:50, dschuyler wrote:
> Would it make sense to have this as 
> list-item[selected] {
>   font-weight: 500;
> }
> in settings_share_css? (or is there any reason
> not to have it there)?

Done (also corrected to .selected)

https://codereview.chromium.org/1856873002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/manage_languages_page.html
(right):

https://codereview.chromium.org/1856873002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/manage_languages_page.html:26:
<div  class="start">
On 2016/04/06 21:45:50, dschuyler wrote:
> Is that extra space intentional?

Done.

https://codereview.chromium.org/1856873002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/manage_languages_page.html:28:
<div id="languages-list" class="list-frame vertical-list">
On 2016/04/06 21:45:50, dschuyler wrote:
> The style doc (below) suggests using camel
> case id names. 
> 
>
https://docs.google.com/document/d/1G0t519y_cSW4etXVwruR145abZH0N5-9R7JyejzVo...

Removed.

https://codereview.chromium.org/1856873002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/manage_languages_page.html:47:
disabled="[[!canRemoveLanguage_(item.code, prefs.intl.app_locale.value)]]"
On 2016/04/06 21:45:50, dschuyler wrote:
> I think we are now able to wrap these lines,
> can this line be wrapped?

Done. Not sure whether they should get an extra 4 spaces, though.

[michaelpg, 2016-04-07 16:26:36]

Description was changed from

==========
MD Settings: Polish languages pages.

Fix layout and formatting issues and address updated mocks
Use real values for secondary labels
Link to language detail page from spellcheck items
Minor performance enhancements
==========

to

==========
MD Settings: Polish languages pages.

Fix layout and formatting issues and address updated mocks
Use real values for secondary labels
Link to language detail page from spellcheck items
Minor performance enhancements

BUG=512479
==========

[dschuyler, 2016-04-07 19:24:01]

LGTM with a nit for @private and a question about a css class.

https://codereview.chromium.org/1856873002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.js (right):

https://codereview.chromium.org/1856873002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:60: * @type
{!LanguageInfo|undefined}
@private

https://codereview.chromium.org/1856873002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/manage_languages_page.html
(right):

https://codereview.chromium.org/1856873002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/manage_languages_page.html:17:
align-items: initial;
optional, just asking: should any of these be part of
the normal .list-frame class?

[michaelpg, 2016-04-07 23:10:48]

thanks!

https://codereview.chromium.org/1856873002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.js (right):

https://codereview.chromium.org/1856873002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:60: * @type
{!LanguageInfo|undefined}
On 2016/04/07 19:24:01, dschuyler wrote:
> @private

Done.

https://codereview.chromium.org/1856873002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/manage_languages_page.html
(right):

https://codereview.chromium.org/1856873002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/manage_languages_page.html:17:
align-items: initial;
On 2016/04/07 19:24:01, dschuyler wrote:
> optional, just asking: should any of these be part of
> the normal .list-frame class?

Don't think so, it's a different layout. I'm only using it for these:

        -webkit-padding-end: 20px;
        -webkit-padding-start: 48px;

Add it to my gripes about the layout classes ;-)

[michaelpg, 2016-04-08 19:09:37]

landing

[michaelpg, 2016-04-08 19:09:44]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-04-08 19:09:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/1856873002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-08 19:10:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1856873002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1856873002/160001

[commit-bot: I haz the power, 2016-04-08 19:56:40]

Description was changed from

==========
MD Settings: Polish languages pages.

Fix layout and formatting issues and address updated mocks
Use real values for secondary labels
Link to language detail page from spellcheck items
Minor performance enhancements

BUG=512479
==========

to

==========
MD Settings: Polish languages pages.

Fix layout and formatting issues and address updated mocks
Use real values for secondary labels
Link to language detail page from spellcheck items
Minor performance enhancements

BUG=512479
==========

[commit-bot: I haz the power, 2016-04-08 19:56:41]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-04-08 20:01:46]

Description was changed from

==========
MD Settings: Polish languages pages.

Fix layout and formatting issues and address updated mocks
Use real values for secondary labels
Link to language detail page from spellcheck items
Minor performance enhancements

BUG=512479
==========

to

==========
MD Settings: Polish languages pages.

Fix layout and formatting issues and address updated mocks
Use real values for secondary labels
Link to language detail page from spellcheck items
Minor performance enhancements

BUG=512479

Committed: https://crrev.com/4d04ee92fa89e743089f6547a46015a1bb27b514
Cr-Commit-Position: refs/heads/master@{#386169}
==========

[commit-bot: I haz the power, 2016-04-08 20:01:47]

Patchset 7 (id:??) landed as
https://crrev.com/4d04ee92fa89e743089f6547a46015a1bb27b514
Cr-Commit-Position: refs/heads/master@{#386169}