chrome/browser/resources/ntp4/new_tab.html - Issue 12985002: Add a new App launcher promo to the apps page / NTP.

Side by Side Diff: chrome/browser/resources/ntp4/new_tab.html

Issue 12985002: Add a new App launcher promo to the apps page / NTP. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: FieldTrial CR Comments 1 + one more test fix. Created 7 years, 9 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
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <html i18n-values="	2 <html i18n-values="

3 dir:textdirection;	3 dir:textdirection;

4 hasattribution:hasattribution;	4 hasattribution:hasattribution;

5 themegravity:themegravity;	5 themegravity:themegravity;

6 bookmarkbarattached:bookmarkbarattached;"	6 bookmarkbarattached:bookmarkbarattached;"

7 class="starting-up">	7 class="starting-up">

8 <head>	8 <head>

9 <meta charset="utf-8">	9 <meta charset="utf-8">

10 <title i18n-content="title"></title>	10 <title i18n-content="title"></title>

(...skipping 75 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
86 <div id="page-list"></div>	86 <div id="page-list"></div>

87 <button id="page-switcher-end" class="page-switcher custom-appearance"	87 <button id="page-switcher-end" class="page-switcher custom-appearance"

88 tabindex="2" hidden>›	88 tabindex="2" hidden>›

89 </button>	89 </button>

90 <div id="attribution">	90 <div id="attribution">

91 <span i18n-content="attributionintro"></span>	91 <span i18n-content="attributionintro"></span>

92 <img id="attribution-img">	92 <img id="attribution-img">

93 </div>	93 </div>

94 </div>	94 </div>

95	95

	96 <div class="app-launcher-promo" id="app-launcher-promo" hidden>
	arv (Not doing code reviews) 2013/03/26 14:02:39 You should not have both id and class that have th You should not have both id and class that have the same value. Classes are usually used when you want to classify multiple elements and id is used to identify a single element. In this case it seems like you can remove the class. MAD 2013/03/26 15:42:43 But I need the class for the CSS stuff... no? And Show quoted text On 2013/03/26 14:02:39, arv wrote: > You should not have both id and class that have the same value. Classes are > usually used when you want to classify multiple elements and id is used to > identify a single element. In this case it seems like you can remove the class. But I need the class for the CSS stuff... no? And I need the ID to access it in JS and show/hide it. arv (Not doing code reviews) 2013/03/26 15:53:34 You can use an ID selector in the CSS. #app-launc Show quoted text On 2013/03/26 15:42:43, MAD wrote: > On 2013/03/26 14:02:39, arv wrote: > > You should not have both id and class that have the same value. Classes are > > usually used when you want to classify multiple elements and id is used to > > identify a single element. In this case it seems like you can remove the > class. > > But I need the class for the CSS stuff... no? > And I need the ID to access it in JS and show/hide it. You can use an ID selector in the CSS. #app-launcher-promo { ... } MAD 2013/03/26 17:02:46 Done. Show quoted text On 2013/03/26 15:53:34, arv wrote: > On 2013/03/26 15:42:43, MAD wrote: > > On 2013/03/26 14:02:39, arv wrote: > > > You should not have both id and class that have the same value. Classes are > > > usually used when you want to classify multiple elements and id is used to > > > identify a single element. In this case it seems like you can remove the > > class. > > > > But I need the class for the CSS stuff... no? > > And I need the ID to access it in JS and show/hide it. > > You can use an ID selector in the CSS. > > #app-launcher-promo { > ... > } Done.
	97 <div class="apps-promo-text" id="apps-promo-text">
	arv (Not doing code reviews) 2013/03/25 15:04:38 Why do you need div/div/span? Can one element be u Why do you need div/div/span? Can one element be used instead of 3? MAD 2013/03/25 15:32:24 I followed the patterns above... Show quoted text On 2013/03/25 15:04:38, arv wrote: > Why do you need div/div/span? Can one element be used instead of 3? I followed the patterns above... arv (Not doing code reviews) 2013/03/26 14:02:39 The app-launcher-promo is needed to do the centeri Show quoted text On 2013/03/25 15:32:24, MAD wrote: > On 2013/03/25 15:04:38, arv wrote: > > Why do you need div/div/span? Can one element be used instead of 3? > > I followed the patterns above... The app-launcher-promo is needed to do the centering. However, the inner span can be removed I believe. MAD 2013/03/26 15:42:43 Done. Show quoted text On 2013/03/26 14:02:39, arv wrote: > On 2013/03/25 15:32:24, MAD wrote: > > On 2013/03/25 15:04:38, arv wrote: > > > Why do you need div/div/span? Can one element be used instead of 3? > > > > I followed the patterns above... > > The app-launcher-promo is needed to do the centering. However, the inner span > can be removed I believe. Done.
	98 <span i18n-content="appsPromoTitle"></span>

	99 </div>

	100 <a href="https://chrome.google.com/webstore/launcher">
	arv (Not doing code reviews) 2013/03/25 15:04:38 Same here? Same here? MAD 2013/03/25 15:32:24 Done. Show quoted text On 2013/03/25 15:04:38, arv wrote: > Same here? Done.
	101 <div class="apps-promo-learn-more" id="apps-promo-learn-more">

	102 <span i18n-content="learn_more"></span>

	103 </div>

	104 </a>

	105 <img src="app_launcher_promo.png"/>
	arv (Not doing code reviews) 2013/03/25 15:04:38 No /> in HTML No /> in HTML MAD 2013/03/25 15:32:24 Done. Show quoted text On 2013/03/25 15:04:38, arv wrote: > No /> in HTML Done.
	106 <div class="close-button" id="app-launcher-promo-close-button"></div>
	arv (Not doing code reviews) 2013/03/25 15:04:38 Should this be a <button>? What is the accessibili Should this be a <button>? What is the accessibility story here? MAD 2013/03/25 15:32:24 Everywhere else I saw the close-button, it was in Show quoted text On 2013/03/25 15:04:38, arv wrote: > Should this be a <button>? What is the accessibility story here? Everywhere else I saw the close-button, it was in a div. arv (Not doing code reviews) 2013/03/26 14:02:39 Really? Look at the #notification above: <button Show quoted text On 2013/03/25 15:32:24, MAD wrote: > On 2013/03/25 15:04:38, arv wrote: > > Should this be a <button>? What is the accessibility story here? > > Everywhere else I saw the close-button, it was in a div. Really? Look at the #notification above: <button class="close-button custom-appearance" class="custom-appearance"> MAD 2013/03/26 15:42:43 Ho, sorry, I didn't notice that one... :-/ Show quoted text On 2013/03/26 14:02:39, arv wrote: > On 2013/03/25 15:32:24, MAD wrote: > > On 2013/03/25 15:04:38, arv wrote: > > > Should this be a <button>? What is the accessibility story here? > > > > Everywhere else I saw the close-button, it was in a div. > > Really? Look at the #notification above: > > <button class="close-button custom-appearance" class="custom-appearance"> Ho, sorry, I didn't notice that one... :-/
	107 </div>

	108

96 <div id="footer">	109 <div id="footer">

97 <div id="footer-border"></div>	110 <div id="footer-border"></div>

98 <div id="footer-content">	111 <div id="footer-content">

99 <div id="logo-img">	112 <div id="logo-img">

100 <img alt="" src="chrome://theme/IDR_PRODUCT_LOGO">	113 <img alt="" src="chrome://theme/IDR_PRODUCT_LOGO">

101 <div id="promo-bubble-anchor"></div>	114 <div id="promo-bubble-anchor"></div>

102 </div>	115 </div>

103	116

104 <ul id="dot-list">	117 <ul id="dot-list">

105 </ul>	118 </ul>

(...skipping 80 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
186 </a>	199 </a>

187 </p>	200 </p>

188 </div>	201 </div>

189	202

190 </div>	203 </div>

191	204

192 <!-- This is used to measure text in the current locale. It is not visible. -->	205 <!-- This is used to measure text in the current locale. It is not visible. -->

193 <div id="fontMeasuringDiv"></div>	206 <div id="fontMeasuringDiv"></div>

194	207

195 </html>	208 </html>

OLD	NEW

« chrome/browser/resources/ntp4/apps_page.css ('K') | « chrome/browser/resources/ntp4/apps_page.css ('k') | chrome/browser/resources/ntp4/new_tab.js » ('j') | no next file with comments »