Chromium Code Reviews Chromium Code Reviews
Issue 2760033003:
Modernize web development style guide (Closed)
| OLD | NEW |
|---|---|
| (Empty) | |
| 1 <style> | |
| 2 | |
| 3 .note::before { | |
| 4 content: 'Note: '; | |
| 5 font-variant: small-caps; | |
| 6 font-style: italic; | |
| 7 } | |
| 8 | |
| 9 </style> | |
| 10 | |
| 11 # Chromium Web Development Style Guide | |
| 12 | |
| 13 [TOC] | |
| 14 | |
| 15 ## Where does this style guide apply? | |
| 16 | |
| 17 This style guide targets Chromium frontend features implemented with JavaScript, | |
| 18 CSS, and HTML. Developers of these features should adhere to the following | |
| 19 rules where possible, just like those using C++ conform to the [Chromium C++ | |
| 20 styleguide](../c++/c++.md). | |
| 21 | |
| 22 This guide follows and builds on: | |
| 23 | |
| 24 * [Google HTML/CSS Style Guide](https://google.github.io/styleguide/htmlcssguide .xml) | |
| 25 * [Google JavaScript Style Guide](https://google.github.io/styleguide/javascript guide.xml) | |
| 26 * [Google Polymer Style Guide](http://go/polymer-style) | |
| 27 | |
| 28 <div class="note"> | |
| 29 Concerns for browser compatibility are usually not relevant for Chromium-only | |
| 30 code. | |
| 31 </div> | |
| 32 | |
| 33 ## Separation of presentation and content | |
| 34 | |
| 35 When designing a feature with web technologies, separate the: | |
| 36 | |
| 37 * **content** you are presenting to the user (**HTML**) | |
| 38 * **styling** of the data (**CSS**) | |
| 39 * **logic** that controls the dynamic behavior of the content and presentation | |
| 40 (**JS**) | |
| 41 | |
| 42 This highlights the concern of each part of the code and promotes looser | |
| 43 coupling (which makes refactor easier down the road). | |
| 44 | |
| 45 Another way to envision this principle is using the MVC pattern: | |
| 46 | |
| 47 | MVC Component | Web Component | | |
| 48 |:-------------:|:-------------:| | |
| 49 | Model | HTML | | |
| 50 | View | CSS | | |
| 51 | Controller | JS | | |
| 52 | |
| 53 It's also often appropriate to separate each implementation into separate files. | |
| 54 | |
| 55 DO: | |
| 56 ```html | |
| 57 <!-- missile-button.html --> | |
| 58 <link rel="stylesheet" href="warnings.css"> | |
| 59 <b class="warning">LAUNCH BUTTON WARNING</b> | |
| 60 <script src="missile-button.js"> | |
| 61 ``` | |
| 62 ```css | |
| 63 /* warnings.css */ | |
| 64 .warning { | |
| 65 color: red; | |
| 66 } | |
| 67 ``` | |
| 68 ```js | |
| 69 // missile-button.js | |
| 70 document.querySelector('b').onclick = fireZeeMissiles; | |
| 71 ``` | |
| 72 | |
| 73 DON'T: | |
| 74 ```html | |
| 75 <!-- missile-button.html --> | |
| 76 <b style="color: red;" onclick="fireZeeMissiles()">LAUNCH BUTTON WARNING</span> | |
| 77 ``` | |
| 78 | |
| 79 <div class="note"> | |
| 80 For various technical and historical reasons, code using the Polymer library may | |
| 81 use <code>on-event</code>-style event listener wiring and | |
| 82 <code><style></code> tags that live inside of .html files. | |
| 83 </div> | |
| 84 | |
| 85 ## HTML | |
| 86 | |
| 87 See the [Google HTML/CSS Style | |
| 88 guide](https://google.github.io/styleguide/htmlcssguide.xml). | |
| 89 | |
| 90 ### Head | |
| 91 | |
| 92 ```html | |
| 93 <!doctype html> | |
| 94 <html dir="$i18n{direction}"> | |
| 95 <head> | |
| 96 <meta charset="utf-8"> | |
| 97 <title>$i18n{myFeatureTitle}</title> | |
| 98 <link rel="icon" href="feature.png"> | |
| 99 <link rel="stylesheet" href="feature.css"> | |
| 100 <script src="feature.js"></script> | |
| 101 </head> | |
| 102 … | |
| 103 </html> | |
| 104 ``` | |
| 105 | |
| 106 * Specify `<!doctype html>`. | |
| 107 | |
| 108 * Set the `dir` attribute of the html element to the localized ‘textdirection’ | |
| 109 value. This flips the page visually for RTL languages and allows | |
| 110 `html[dir=rtl]` selectors to work. | |
| 111 | |
| 112 * Specify the charset, UTF-8. | |
| 113 | |
| 114 * Link in image, icon and stylesheet resources. | |
| 115 * Do not style elements with `style="..."` attributes. | |
| 116 | |
| 117 * Include the appropriate JS scripts. | |
| 118 * Do not add JS to element event handlers. | |
| 119 | |
| 120 <div class="note"> | |
| 121 Polymer event handlers like <code>on-tap</code> are allowed and often reduce the | |
| 122 amount of addressing (adding an ID just to wire up event handling). | |
| 123 </div> | |
| 124 | |
| 125 ### Body | |
| 126 | |
| 127 ```html | |
| 128 <h3>$i18n{autofillAddresses}</h3> | |
| 129 <div class="settings-list"> | |
| 130 <list id="address-list"></list> | |
| 131 <div> | |
| 132 <button id="autofill-add-address">$i18n{autofillAddAddress}</button> | |
| 133 </div> | |
| 134 </div> | |
| 135 <if expr="chromeos"> | |
| 136 <a href="https://www.google.com/support/chromeos/bin/answer.py?answer=142893" | |
| 137 target="_blank">$i18n{learnMore}</a> | |
| 138 </if> | |
| 139 ``` | |
| 140 | |
| 141 * Element IDs use `dash-form` | |
| 142 * Exception: `camelCase` is allowed in Polymer code for easier | |
|
michaelpg
2017/03/21 18:02:15
indent off
Dan Beam
2017/03/21 20:45:42
Done.
| |
| 143 `this.$.idName` access. | |
| 144 | |
| 145 * Localize all strings using $i18n{} | |
| 146 | |
| 147 * Use camelCase for $i18n{} keys names. | |
| 148 | |
| 149 * Add 2 space indentation in each new block. | |
| 150 | |
| 151 * Adhere to the 80-column limit. | |
| 152 * Indent 4 spaces when wrapping a previous line. | |
| 153 | |
| 154 * Use double-quotes instead of single-quotes for all attributes. | |
| 155 | |
| 156 * Don't close single tags | |
| 157 * DO: `<input type="radio">` | |
| 158 * DON'T: `<input type="radio" />` | |
| 159 | |
| 160 <div class="note"> | |
| 161 All <code><custom-elements></code> and some HTML elements like | |
| 162 <code><iframe></code> require closing. | |
| 163 </div> | |
| 164 | |
| 165 * Use the `button` element instead of `<input type="button">`. | |
| 166 | |
| 167 * Do not use `<br>`; place blocking elements (`<div>`) as appropriate. | |
| 168 | |
| 169 * Do not use spacing-only divs; set the margins on the surrounding elements. | |
| 170 | |
| 171 * Only use `<table>` elements when displaying tabular data. | |
| 172 | |
| 173 * Do not use the `for` attribute of `<label>` | |
| 174 * If you're labelling a checkbox, put the `<input>` inside the `<label>` | |
| 175 * If you're labelling purely for accessibility, e.g. a `<select> `, use | |
|
michaelpg
2017/03/21 18:02:16
indent off?
Dan Beam
2017/03/21 20:45:42
Done.
| |
| 176 `aria-labelledby` | |
| 177 | |
| 178 ## CSS | |
| 179 | |
| 180 See the [Google HTML/CSS style | |
| 181 guide](https://google.github.io/styleguide/htmlcssguide.xml) (and again, browser | |
| 182 compatibility issues are less relevant for Chrome-only code). | |
| 183 | |
| 184 ```css | |
| 185 .raw-button, | |
| 186 .raw-button:hover, | |
| 187 .raw-button:active { | |
| 188 --sky-color: blue; | |
| 189 -webkit-margin-start: 0; | |
| 190 background-color: rgb(253, 123, 42); | |
| 191 background-repeat: no-repeat; | |
| 192 border: none; | |
| 193 min-width: 0; | |
| 194 padding: 1px 6px; | |
| 195 } | |
| 196 ``` | |
| 197 | |
| 198 * Specify one selector per line. | |
| 199 * Exception: One rule / one line frames in a `@keyframe` (see below). | |
| 200 | |
| 201 * Opening brace on the same line as the last (or only) selector. | |
| 202 | |
| 203 * Two-space indentation for each declaration, one declaration per line, | |
| 204 terminated by a semicolon. | |
| 205 | |
| 206 * Use shorthand notation when possible. | |
| 207 | |
| 208 * Alphabetize properties. | |
| 209 * `-webkit` properties should be listed at the top, sorted alphabetically. | |
| 210 * `--variables` and `--mixins: {}` should be alphabetically declared when | |
| 211 possible. | |
| 212 | |
| 213 * Insert a space after the colon separating property and value. | |
| 214 | |
| 215 * Do not create a class for only one element; use the element ID instead. | |
| 216 | |
| 217 * When specifying length values, do not specify units for a zero value, e.g., | |
| 218 `left: 0px;` becomes `left: 0;` | |
| 219 * Exception: 0% values in lists of percentages like `hsl(5, 0%, 90%)` or | |
| 220 within @keyframe directives, e.g: | |
| 221 ```css | |
| 222 @keyframe animation-name { | |
| 223 0% { /* beginning of animation */ } | |
| 224 100% { /* end of animation */ } | |
| 225 } | |
| 226 ``` | |
| 227 | |
| 228 * Use single quotes instead of double quotes for all strings. | |
| 229 | |
| 230 * Don't use quotes around `url()`s unless needed (i.e. a `data:` URI). | |
| 231 | |
| 232 * Class names use `dash-form`. | |
| 233 | |
| 234 * If time lengths are less than 1 second, use millisecond granularity. | |
| 235 * DO: `transition: height 200ms;` | |
| 236 * DON'T: `transition: height 0.2s;` | |
| 237 | |
| 238 * Use two colons when addressing a pseudo-element (i.e. `::after`, `::before`, | |
| 239 `::-webkit-scrollbar`). | |
| 240 | |
| 241 * Use scalable `font-size` units like `%` or `em` to respect users' default font | |
| 242 size | |
| 243 | |
| 244 * Use `*-top/bottom` instead of `-webkit-*-before/after` | |
| 245 * `-top/bottom` are easier to understand (`before/after` is confusingly | |
| 246 similar to `start/end`) | |
| 247 * `-webkit-*-before/after` has far less advantage than `-webkit-*-start/end` | |
| 248 | |
| 249 ### Color | |
| 250 | |
| 251 * When possible, use named colors (i.e. `white`, `black`) to enhance | |
| 252 readability. | |
| 253 | |
| 254 * Prefer `rgb()` or `rgba()` with decimal values instead of hex notation | |
| 255 (`#rrggbb`) because alpha can be more easily added. | |
| 256 * Exception: shades of gray (i.e. `#333`) | |
| 257 | |
| 258 * If the hex value is `#rrggbb`, use the shorthand notation `#rgb`. | |
| 259 | |
| 260 ### URLs | |
| 261 | |
| 262 * Don't embed data URIs in source files. Instead, use grit's flattening. | |
| 263 | |
| 264 ```css | |
| 265 background-image: url(../path/to/image.svg); | |
| 266 ``` | |
| 267 | |
| 268 The contents of file.png are base64-encoded and the `url()` is replaced with | |
| 269 | |
| 270 ```css | |
| 271 background-image: url(data:image/xml+svg;base64,...); | |
| 272 ``` | |
| 273 | |
| 274 if `flattenhtml="true"` is specified in your .grd file. | |
| 275 | |
| 276 ### RTL | |
| 277 | |
| 278 ```css | |
| 279 .suboption { | |
| 280 -webkit-margin-start: 16px; | |
| 281 } | |
| 282 | |
| 283 #save-button { | |
| 284 color: #fff; | |
| 285 left: 10px; | |
| 286 } | |
| 287 | |
| 288 html[dir='rtl'] #save-button { | |
| 289 right: 10px; | |
| 290 } | |
| 291 ``` | |
| 292 | |
| 293 Use RTL-friendly versions of things like `margin` or `padding` where possible: | |
| 294 | |
| 295 * `margin-left` -> `-webkit-margin-start` | |
| 296 * `padding-right` -> `-webkit-padding-end` | |
| 297 * `text-align: left` -> `text-align: start` | |
| 298 * `text-align: right` -> `text-align: end` | |
| 299 * set both `left` for `[dir='ltr']` and `right` for `[dir='rtl']` | |
| 300 | |
| 301 For properties that don't have an RTL-friendly alternatives, use | |
| 302 `html[dir='rtl']` as a prefix in your selectors. | |
| 303 | |
| 304 ## JavaScript | |
| 305 | |
| 306 ### Style | |
| 307 | |
| 308 See the [Google JavaScript Style | |
| 309 Guide](https://google.github.io/styleguide/javascriptguide.xml). | |
| 310 | |
| 311 * Use `$('element-id')` instead of `document.getElementById` | |
| 312 | |
| 313 * Use single-quotes instead of double-quotes for all strings. | |
| 314 * `clang-format` now handles this automatically. | |
| 315 | |
| 316 * Omit curly braces for single-line if statements. | |
| 317 | |
| 318 * Use ES5 getters and setters | |
| 319 * Use `@type` (instead of `@return` or `@param`) for JSDoc annotations on | |
| 320 getters/setters | |
| 321 | |
| 322 * See [Annotating JavaScript for the Closure | |
| 323 Compiler](https://developers.google.com/closure/compiler/docs/js-for-compiler) | |
| 324 for @ directives | |
| 325 | |
| 326 * Prefer `event.preventDefault()` to `return false` from event handlers | |
| 327 | |
| 328 ### Closure compiler | |
| 329 | |
| 330 * Use the [closure | |
| 331 compiler](https://chromium.googlesource.com/chromium/src/+/master/docs/closure _compilation.md) | |
| 332 to identify JS type errors and enforce correct JSDoc annotations. | |
| 333 | |
| 334 * Add a `compiled_resources2.gyp` file to any new web UI code directory. | |
| 335 | |
| 336 * Ensure that your `compiled_resources2.gyp` file is included in | |
| 337 `third_party/closure_compiler/compiled_resources2.gyp` (or somewhere in its | |
| 338 include hierarchy) so that your code is typechecked in an automated way. | |
| 339 | |
| 340 * Type Polymer elements by appending 'Element' to the element name, e.g. | |
| 341 `/** @type {IronIconElement} */` | |
| 342 | |
| 343 * Use explicit nullability in JSDoc type information | |
| 344 * Rather than `@type {Object}` use: | |
| 345 * `{!Object}` for only Object | |
| 346 * `{!Object|undefined}` for an Object that may be undefined | |
| 347 * `{?Object}` for Object that may be null | |
| 348 * Do the same for typedefs and Array (or any other nullable type) | |
| 349 | |
| 350 * Don't add a `.` after template types | |
| 351 * DO: `Array<number>` | |
| 352 * DON'T: `Array.<number>` | |
| 353 | |
| 354 * Don't specify string in template object types. That's the only type of key | |
| 355 `Object` can possibly have. | |
| 356 * DO: `Object<T>` | |
| 357 * DON'T: `Object<string, T>` | |
| 358 | |
| 359 ### Events | |
| 360 | |
| 361 * Use Polymer's `on-tap` for click events instead of `on-click` | |
| 362 * `on-tap` handlers should use `stopPropagation()` to prevent parents from | |
| 363 handling the event where appropriate. | |
| 364 | |
| 365 <div class="note"> | |
| 366 Calling <code>stopPropagation()</code> from an <code>on-tap</code> handler will | |
| 367 not prevent on-click event handlers, so make sure that <i>on-tap</i> is used | |
| 368 consistently throughout the page. | |
| 369 </div> | |
| 370 | |
| 371 ## Polymer | |
| 372 | |
| 373 Also see the [Google Polymer Style Guide](http://go/polymer-style). | |
| 374 | |
| 375 * Use a consistent ordering in the “prototype” object passed to `Polymer()`: | |
| 376 * `is` | |
| 377 * `behaviors` | |
|
michaelpg
2017/03/21 18:02:16
etc
Dan Beam
2017/03/21 20:45:42
Done.
| |
| 378 * `properties` (public, then private) | |
| 379 * `hostAttributes` | |
| 380 * `listeners`, `observers` | |
| 381 * `created`, `ready`, `attached`, `detached` | |
| 382 * public methods | |
| 383 * event handlers, computed functions, and private methods | |
| 384 | |
| 385 * Use camelCase for element IDs to simplify local DOM accessors (i.e. | |
| 386 `this.$.camelCase` instead of `this.$[‘dash-case’]`). | |
| 387 | |
| 388 * Use `this.foo` instead of `newFoo` arguments in observers when possible. | |
| 389 This makes changing the type of `this.foo` easier (as the `@type` is | |
|
michaelpg
2017/03/21 18:02:16
spacing intentional?
Dan Beam
2017/03/21 20:45:42
no, all of this is because paste mode
| |
| 390 duplicated in less places, i.e. `@param`). | |
| 391 | |
| 392 ```js | |
| 393 properties: { | |
| 394 foo: {type: Number, observer: 'fooChanged_'} | |
| 395 }, | |
| 396 | |
| 397 /** @private */ | |
| 398 fooChanged_: function() { | |
| 399 this.bar = this.derive(this.foo); | |
| 400 }, | |
| 401 ``` | |
| 402 | |
| 403 ## Grit processing | |
| 404 | |
| 405 Grit is a tool that runs at compile time to pack resources together into | |
| 406 Chromium. | |
| 407 | |
| 408 ### Preprocessing | |
| 409 | |
| 410 Grit can be used to selectively include or exclude code at compile-time in web | |
| 411 code. Preprocessing is be enabled by adding the `preprocess="true"` attribute | |
| 412 inside of a `.grd` file on `<structure>` and `<include>` nodes. | |
| 413 | |
| 414 <div class="note"> | |
| 415 These preprocesor statements can live in places that surprise linters or | |
| 416 formatters (for example: running clang-format on a .js file with an <if> | |
| 417 in it). Generally, putting these language-invalid features inside of comments | |
| 418 helps alleviate problems with unexpected input. | |
| 419 </div> | |
| 420 | |
| 421 `<if>` tags allow conditional logic by evaluating an expression in a | |
| 422 compile-time environment of grit variables. These allow conditionally include | |
| 423 or excluding code. | |
| 424 | |
| 425 Example: | |
| 426 ```js | |
| 427 function isWindows() { | |
| 428 // <if expr="win"> | |
| 429 return true; | |
| 430 // </if> | |
| 431 return false; | |
| 432 } | |
| 433 ``` | |
| 434 | |
| 435 `<include src="[path]">` reads the file at `path` and replaces the `<include>` | |
| 436 tag with the file contents of `[path]`. | |
| 437 | |
| 438 Don't use `</include>` to close these tags; they're not needed nor supported. | |
| 439 | |
| 440 <div class="note"> | |
| 441 Using <code><include></code> simply pastes the entire contents of a file, | |
| 442 which can lead to duplication. If you simply want to ensure some code is loaded | |
| 443 (and usually you do), you should use HTML Imports instead. | |
| 444 </div> | |
| 445 | |
| 446 Grit can read and inline resources when enabled via `flattenhtml="true"`. | |
| 447 | |
| 448 <div class="note"> | |
| 449 The implementation of flattening does HTML parsing and URL detection via regular | |
| 450 expresions and is not guaranteed to work in all cases. | |
| 451 </div> | |
| 452 | |
| 453 Example: | |
| 454 | |
| 455 ```css | |
| 456 .spinner { | |
| 457 background: url(../relative/file/path/to/spinner.svg); | |
| 458 } | |
| 459 ``` | |
| 460 | |
| 461 Is transformed to: | |
| 462 | |
| 463 ```css | |
| 464 .spinner { | |
| 465 background: url(data:image/svg+xml;... base64-encoded content ...); | |
| 466 } | |
| 467 ``` | |
| 468 | |
| 469 A minification tool can be specified to Grit (like Closure compiler) to | |
| 470 transform the code before it's packed into a bundle. | |
| OLD | NEW |
