Skip to content

Commit 58390c6

Browse files
keithamuskeithamus
authored
Merge pull request #45 from github/docs-add-conventions-patterns-anti-patterns
docs: add conventions, patterns, anti-patterns
2 parents 1fdca1c + 60d0b84 commit 58390c6

3 files changed

Lines changed: 415 additions & 0 deletions

File tree

‎docs/_guide/anti-patterns.md‎

Lines changed: 305 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,305 @@
1+
---
2+
chapter: 9
3+
subtitle: Anti Patterns
4+
---
5+
6+
{% capture discouraged %}<h4 class="text-red"><svg class="octicon" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24"><path fill-rule="evenodd" d="M1 12C1 5.925 5.925 1 12 1s11 4.925 11 11-4.925 11-11 11S1 18.075 1 12zm8.036-4.024a.75.75 0 00-1.06 1.06L10.939 12l-2.963 2.963a.75.75 0 101.06 1.06L12 13.06l2.963 2.964a.75.75 0 001.061-1.06L13.061 12l2.963-2.964a.75.75 0 10-1.06-1.06L12 10.939 9.036 7.976z"></path></svg> Discouraged</h4>{% endcapture %}
7+
8+
{% capture encouraged %}<h4 class="text-green"><svg class="octicon" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24"><path fill-rule="evenodd" d="M1 12C1 5.925 5.925 1 12 1s11 4.925 11 11-4.925 11-11 11S1 18.075 1 12zm16.28-2.72a.75.75 0 00-1.06-1.06l-5.97 5.97-2.47-2.47a.75.75 0 00-1.06 1.06l3 3a.75.75 0 001.06 0l6.5-6.5z"></path></svg> Encouraged</h4>{% endcapture %}
9+
10+
Here are a few common anti-patterns which we've discovered as developers have used Catalyst. We consider these anti-patterns as they're best avoided, because of surprising edge-cases, or simply because there are easier ways to achieve the same goals.
11+
12+
### Avoid doing any initialisation in the constructor
13+
14+
With conventional classes, it is expected that initialisation will be done in the `constructor()` method. Custom Elements are slightly different, because the `constructor` is called _before_ the element has been put into the Document, which means any initialisation that expects to be connected to a DOM will fail.
15+
16+
{{ discouraged }}
17+
18+
```typescript
19+
import { controller } from "@github/catalyst"
20+
21+
@controller
22+
class HelloWorldElement extends HTMLElement {
23+
constructor() {
24+
// This will fire before DOM is connected, so will never bubble!
25+
this.dispatchEvent(new CustomEvent('loaded'))
26+
}
27+
}
28+
```
29+
30+
{{ encouraged }}
31+
32+
```typescript
33+
import { controller } from "@github/catalyst"
34+
35+
@controller
36+
class HelloWorldElement extends HTMLElement {
37+
connectedCallback() {
38+
// This will fire _after_ DOM is connected, so will bubble up as expected
39+
this.dispatchEvent(new CustomEvent('loaded'))
40+
}
41+
}
42+
```
43+
44+
45+
### Avoid interacting with parents, use Events where possible
46+
47+
Sometimes it's necessary to let ancestors know about the state of a child element, for example when an element loads or needs the parent to change somehow. Sometimes it can be tempting to use methods like `this.closest()` to get a reference to the parent element and interact with it directly, but this creates a fragile coupling to elements and is best avoided. Events can used here, instead:
48+
49+
{{ discouraged }}
50+
51+
<div class="d-flex my-4">
52+
<div class="">
53+
54+
```typescript
55+
import { controller } from "@github/catalyst"
56+
57+
@controller
58+
class UserSettingsElement extends HTMLElement {
59+
loading() {
60+
// While this is loading we need to disable
61+
// the whole User if `user-profile` ever
62+
// changes, this code will break!
63+
this
64+
.closest('user-profile')
65+
.disable()
66+
}
67+
}
68+
```
69+
70+
</div><div class="ml-4">
71+
72+
```html
73+
<user-profile>
74+
<user-settings></user-settings>
75+
</user-profile>
76+
```
77+
78+
</div>
79+
</div>
80+
81+
Instead of interacting with the parent's API directly in JS, you can use `Events` which can be listened to with `data-action`, this moves any coupling into the HTML which already has the association, and so subsequent refactors will have far less risk of breaking the code:
82+
83+
{{ encouraged }}
84+
85+
<div class="d-flex my-4">
86+
<div class="">
87+
88+
```typescript
89+
import { controller } from "@github/catalyst"
90+
91+
@controller
92+
class UserSettingsElement extends HTMLElement {
93+
loading() {
94+
this.dispatchEvent(
95+
new CustomEvent('loading')
96+
)
97+
}
98+
}
99+
```
100+
101+
</div><div class="ml-4">
102+
103+
```html
104+
<user-profile>
105+
<user-settings
106+
data-action="loading:user-profile#disable">
107+
</user-settings>
108+
</user-profile>
109+
```
110+
111+
</div>
112+
</div>
113+
114+
### Avoid naming methods after events, e.g. `onClick`
115+
116+
When you have a method which is only called as an event, it is tempting to name that method based off of the event, e.g. `onClick`, `onInputFocus`, and so on. This name implies a coupling between the event and method, which later refactorings may break. Also names like `onClick` are very close to `onclick` which is already [part of the Element's API](https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onclick). Instead we recommend naming the method after what it does, not how it is called, for example `resetForm`:
117+
118+
{{ discouraged }}
119+
120+
<div class="d-flex my-4">
121+
<div class="">
122+
123+
```js
124+
import { controller } from "@github/catalyst"
125+
126+
@controller
127+
class UserLoginElement extends HTMLElement {
128+
129+
// `onClick` is not clear
130+
onClick() {
131+
// Log the user in
132+
}
133+
}
134+
```
135+
136+
</div>
137+
<div class="ml-4">
138+
139+
```html
140+
<user-login>
141+
<!-- ... -->
142+
<button
143+
data-action="click:user-login#onClick">
144+
<!-- `onClick` is not clear -->
145+
Log In
146+
</button>
147+
</user-login>
148+
```
149+
150+
</div>
151+
</div>
152+
153+
{{ encouraged }}
154+
155+
<div class="d-flex my-4">
156+
<div class="">
157+
158+
```js
159+
import { controller } from "@github/catalyst"
160+
161+
@controller
162+
class UserLoginElement extends HTMLElement {
163+
164+
login() {
165+
// Log the user in
166+
}
167+
}
168+
```
169+
170+
</div>
171+
<div class="ml-4">
172+
173+
```html
174+
<user-login>
175+
<!-- ... -->
176+
<button
177+
data-action="click:user-login#login">
178+
Log In
179+
</button>
180+
</user-login>
181+
```
182+
183+
</div>
184+
</div>
185+
186+
### Avoid querying against your element, use `@target` or `@targets`
187+
188+
We find it very common for developers to return to habits and use `querySelector[All]` when needing to get elements. The `@target` and `@targets` decorators were designed to simplify `querySelector[All]` and avoid certain bugs with them (such as nesting issues, and unnecessary coupling) so it's a good idea to use them as much as possible:
189+
190+
{{ discouraged }}
191+
192+
```typescript
193+
class UserListElement extends HTMLElement {
194+
showAdmins() {
195+
// Just need to get admins here...
196+
for (const user of this.querySelector('[data-is-admin]')) {
197+
user.hidden = false
198+
}
199+
}
200+
}
201+
```
202+
203+
{{ encouraged }}
204+
205+
```typescript
206+
class UserList {
207+
@targets admins!: HTMLElement[]
208+
209+
showAdmins() {
210+
// Just need to get admins here...
211+
for (const user of this.admins) {
212+
user.hidden = false
213+
}
214+
}
215+
}
216+
```
217+
218+
219+
### Avoid filtering `@targets`, use another `@target` or `@targets`
220+
221+
222+
Sometimes you might need to get a subset of elements from a `@targets` selector. When doing this, simply use another `@target` or `@targets` attribute, it's okay to have many of these! Adding getters which simply return a `@targets` subset has various drawbacks which make it an anti pattern.
223+
224+
For example let's say we have a list of filter checkboxes and checking the "all" checkbox unchecks all other checkboxes:
225+
226+
{{ discouraged }}
227+
228+
```typescript
229+
@controller
230+
class UserFilter {
231+
@targets filters!: HTMLInputElement[]
232+
233+
get allFilter() {
234+
return this.filters.find(el => el.matches('[data-filter="all"]'))
235+
}
236+
237+
filter(event: Event) {
238+
if (event.target === this.allFilter) {
239+
for(const filter of this.filters) {
240+
if (filter !== this.allFilter) filter.checked = false
241+
}
242+
}
243+
// ...
244+
}
245+
246+
}
247+
```
248+
249+
```html
250+
<user-list>
251+
<label><input type="checkbox"
252+
data-action="change:user-list.filter"
253+
data-target="user-list.filters"
254+
data-filter="all">Show all</label>
255+
<label><input type="checkbox"
256+
data-action="change:user-list.filter"
257+
data-target="user-list.filters"
258+
data-filter="new">New Users</label>
259+
<label><input type="checkbox"
260+
data-action="change:user-list.filter"
261+
data-target="user-list.filters"
262+
data-filter="admin">Admins</label>
263+
<!-- ... --->
264+
</user-filter>
265+
```
266+
267+
While this works well, it could be more easily solved with targets:
268+
269+
{{ encouraged }}
270+
271+
```typescript
272+
@controller
273+
class UserFilter {
274+
@targets filters!: HTMLInputElement[]
275+
@target allFilter!: HTMLInputElement
276+
277+
filter(event: Event) {
278+
if (event.target === this.allFilter) {
279+
for (const filter of this.filters) {
280+
if (filter !== this.allFilter) filter.checked = false
281+
}
282+
}
283+
// ...
284+
}
285+
286+
}
287+
```
288+
289+
```html
290+
<user-filter>
291+
<label><input type="checkbox"
292+
data-action="change:user-list.filter"
293+
data-target="user-list.filters user-list.allFilter"
294+
data-filter="all">Show all</label>
295+
<label><input type="checkbox"
296+
data-action="change:user-list.filter"
297+
data-target="user-list.filters"
298+
data-filter="new">New Users</label>
299+
<label><input type="checkbox"
300+
data-action="change:user-list.filter"
301+
data-target="user-list.filters"
302+
data-filter="admin">Admins</label>
303+
<!-- ... --->
304+
</user-filter>
305+
```

‎docs/_guide/conventions.md‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
---
2+
chapter: 7
3+
subtitle: Conventions
4+
---
5+
6+
Catalyst strives for convention over code. Here are a few conventions we recommend when writing Catalyst code:
7+
8+
### Use `Element` to suffix your controller class
9+
10+
Built in HTML elements all extend from the `HTMLElement` constructor, and are all suffixed with `Element` (for example `HTMLElement`, `SVGElement`, `HTMLInputElement` and so on). Catalyst components should be no different, they should behave as closely to the built-ins as possible.
11+
12+
```typescript
13+
@controller
14+
class UserListElement extends HTMLElement {}
15+
```
16+
17+
### The best class-names are two word descriptions
18+
19+
Custom elements are required to have a `-` inside the tag name. Catalyst's `@controller` will derive the tag name from the class name - and so as such the class name needs to have at least two capital letters, or to put it another way, it needs to consist of two words. The element name should describe what it does succinctly in two words. Some examples:
20+
21+
- `theme-picker` (`class ThemePickerElement`)
22+
- `markdown-toolbar` (`class MarkdownToolbarElement`)
23+
- `user-list` (`class UserListElement`)
24+
- `content-pager` (`class ContentPagerElement`)
25+
- `image-gallery` (`class ImageGalleryElement`)
26+
27+
If you're struggling to come up with two words, think about one word being the "what" (what does it do?) and another being the "how" (how does it do it?).
28+
29+
### Keep class-names short (but not too short)
30+
31+
Brevity is good, element names are likely to be typed out a lot, especially throughout HTML in as tag names, and `data-target`, `data-action` attributes. A good rule of thumb is to try to keep element names down to less than 15 characters (excluding the `Element` suffix), and ideally less than 10. Also, longer words are generally harder to spell, which means mistakes might creep into your code.
32+
33+
Be careful not to go too short! We'd recommend avoiding contracting words such as using `Img` to mean `Image`. It can create confusion, especially if there are inconsistencies across your code!
34+
35+
### Method names should describe what they do
36+
37+
A good method name, much like a good class name, describes what it does, not how it was invoked. While methods can be given any name, names like `onClick` are best avoided, overly generic names like `toggle` should also be avoided. Just like class names it is a good idea to ask "how" and "what", so for example `showAdmins`, `filterUsers`, `updateURL`.

0 commit comments

Comments
 (0)