Skip to content

Commit d28b799

Browse files
authored
Merge branch 'master' into docs-rewrite-your-first-component
2 parents 385a87f + f8343ad commit d28b799

7 files changed

Lines changed: 91 additions & 23 deletions

File tree

docs/_guide/anti-patterns.md

Lines changed: 64 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,10 @@ chapter: 9
33
subtitle: Anti Patterns
44
---
55

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 %}
6+
{% capture octx %}<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>{% endcapture %}
7+
{% capture octick %}<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>{% endcapture %}
8+
{% capture discouraged %}<h4 class="text-red">{{ octx }} Discouraged</h4>{% endcapture %}
9+
{% capture encouraged %}<h4 class="text-green">{{ octick }} Encouraged</h4>{% endcapture %}
910

1011
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.
1112

@@ -111,6 +112,59 @@ class UserSettingsElement extends HTMLElement {
111112
</div>
112113
</div>
113114

115+
### Avoid shadowing method names
116+
117+
When naming a method, you should avoid naming it something that already exists on the `HTMLElement` prototype; as doing so can lead to surprising behaviors. Test out the form below to see what method names are allowed or not:
118+
119+
<form>
120+
<label>
121+
<h4>I want my method to be called...</h4>
122+
<input class="js-methodname-shadow-test mb-4">
123+
</label>
124+
<div hidden class="js-methodname-shadow-bad-input text-red">
125+
{{ octx }} This name would shadow <code></code>, you'll need to pick a different name
126+
</div>
127+
<div hidden class="js-methodname-shadow-warn-input text-orange-light">
128+
{{ octx }} While this name is allowed, it's not ideal because <span></span>. You should consider a different name.
129+
</div>
130+
<div hidden class="js-methodname-shadow-good-input text-green">
131+
{{ octick }} This is a good name for a method!
132+
</div>
133+
<script>
134+
const warnings = {
135+
'new': 'it has a special meaning in JS',
136+
'super': 'it has a special meaning in JS',
137+
'prototype': 'it has a special meaning in JS',
138+
'requestSubmit': 'it is a proposed new feature',
139+
}
140+
document.querySelector('.js-methodname-shadow-test').addEventListener('input', () => {
141+
const name = event.target.value
142+
const goodEl = document.querySelector('.js-methodname-shadow-good-input')
143+
const badEl = document.querySelector('.js-methodname-shadow-bad-input')
144+
const warnEl = document.querySelector('.js-methodname-shadow-warn-input')
145+
let warning = warnings[name]
146+
if (name !== name.toLowerCase() && name.toLowerCase() in HTMLElement.prototype) {
147+
warning = `it is too similar to \`${name.toLowerCase()}\` which already exists`
148+
} else if (name.startsWith('on') && !(name in HTMLElement.prototype)) {
149+
warning = 'starting with `on` suggests a coupling between the event and the method (see below)'
150+
}
151+
goodEl.hidden = warning || (name in HTMLElement.prototype)
152+
warnEl.hidden = !warning
153+
badEl.hidden = warning || !(name in HTMLElement.prototype)
154+
if (warning) {
155+
warnEl.querySelector('span').textContent = warning
156+
} else if (name in HTMLElement.prototype) {
157+
let proto = HTMLElement.prototype
158+
while(proto !== null) {
159+
if (proto.hasOwnProperty(name)) break
160+
proto = Object.getPrototypeOf(proto)
161+
}
162+
badEl.querySelector('code').textContent = `${proto.constructor.name}.prototype.${name}`
163+
}
164+
})
165+
</script>
166+
</form>
167+
114168
### Avoid naming methods after events, e.g. `onClick`
115169

116170
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`:
@@ -250,15 +304,15 @@ class UserFilter {
250304
<user-list>
251305
<label><input type="checkbox"
252306
data-action="change:user-list.filter"
253-
data-target="user-list.filters"
307+
data-targets="user-list.filters"
254308
data-filter="all">Show all</label>
255309
<label><input type="checkbox"
256310
data-action="change:user-list.filter"
257-
data-target="user-list.filters"
311+
data-targets="user-list.filters"
258312
data-filter="new">New Users</label>
259313
<label><input type="checkbox"
260314
data-action="change:user-list.filter"
261-
data-target="user-list.filters"
315+
data-targets="user-list.filters"
262316
data-filter="admin">Admins</label>
263317
<!-- ... --->
264318
</user-filter>
@@ -290,15 +344,16 @@ class UserFilter {
290344
<user-filter>
291345
<label><input type="checkbox"
292346
data-action="change:user-list.filter"
293-
data-target="user-list.filters user-list.allFilter"
347+
data-target="user-list.allFilter"
348+
data-targets="user-list.filters"
294349
data-filter="all">Show all</label>
295350
<label><input type="checkbox"
296351
data-action="change:user-list.filter"
297-
data-target="user-list.filters"
352+
data-targets="user-list.filters"
298353
data-filter="new">New Users</label>
299354
<label><input type="checkbox"
300355
data-action="change:user-list.filter"
301-
data-target="user-list.filters"
356+
data-targets="user-list.filters"
302357
data-filter="admin">Admins</label>
303358
<!-- ... --->
304359
</user-filter>

docs/_guide/conventions.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,4 +34,8 @@ Be careful not to go too short! We'd recommend avoiding contracting words such a
3434

3535
### Method names should describe what they do
3636

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`.
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 most names, you should avoid names that conflict with existing methods on the `HTMLElement` prototype (more on that in [anti-patterns](/guide/anti-patterns#avoid-shadowing-method-names)). 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`.
38+
39+
### `@target` should use singular naming, while `@targets` should use plural
40+
41+
To help differentiate the two `@target`/`@targets` decorators, the properties should be named with respective to their cardinality. That is to say, if you're using an `@target` decorator, then the name should be singular (e.g. `user`, `field`) while the `@targets` decorator should be coupled with plural property names (e.g. `users`, `fields`).

docs/_guide/targets.md

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ chapter: 5
33
subtitle: Querying Descendants
44
---
55

6-
One of the three [core patterns](/guide/introduction#three-core-concepts-observe-listen-query) is Querying. In Catalyst, Targets are the preferred way to query. Targets use `querySelectorAll` under the hood, but in a way that makes it a lot simpler to work with.
6+
One of the three [core patterns](/guide/introduction#three-core-concepts-observe-listen-query) is Querying. In Catalyst, Targets are the preferred way to query. Targets use `querySelector` under the hood, but in a way that makes it a lot simpler to work with.
77

88
Catalyst Components are really just Web Components, so you could simply use `querySelector` or `querySelectorAll` to select descendants of the element. Targets avoid some of the problems of `querySelector`; they provide a more consistent interface, avoid coupling CSS classes or HTML tag names to JS, and they handle subtle issues like nested components. Targets are also a little more ergonomic to reuse in a class. We'd recommend using Targets over `querySelector` wherever you can.
99

@@ -61,25 +61,25 @@ The target syntax follows a pattern of `controller.target`.
6161
</span>
6262
<div class="p-3">
6363

64-
Remember! There are two decorators available, `@target` which fetches only one element, and `@targets` which fetches multiple. This is the only difference, but it's an important one.
64+
Remember! There are two decorators available, `@target` which fetches only one `data-target` element, and `@targets` which fetches multiple `data-targets` elements!
6565

6666
</div>
6767
</div>
6868

69-
The `@target` decorator will only ever return _one_ element, just like `querySelector`. If you want to get multiple Targets, you need the `@targets` decorator which works almost identically, but it'll return an _array_ of elements. To put this into types: `@target` returns `Element|undefined` while `@targets` returns `Array<Element>`
69+
The `@target` decorator will only ever return _one_ element, just like `querySelector`. If you want to get multiple Targets, you need the `@targets` decorator which works almost the same, but returns an Array of elements, and it searches the `data-targets` attribute (not `data-target`).
7070

7171
Elements can be referenced as multiple targets, and targets may be referenced multiple times within the HTML:
7272

7373
```html
7474
<team-members>
7575
<user-list>
76-
<user-settings data-target="user-list.user">
77-
<input type="checkbox" data-target="team-members.read user-settings.read">
78-
<input type="checkbox" data-target="team-members.write user-settings.write">
76+
<user-settings data-targets="user-list.users">
77+
<input type="checkbox" data-targets="team-members.reads user-settings.reads">
78+
<input type="checkbox" data-targets="team-members.writes user-settings.writes">
7979
</user-settings>
80-
<user-settings data-target="user-list.user">
81-
<input type="checkbox" data-target="team-members.read user-settings.read">
82-
<input type="checkbox" data-target="team-members.write user-settings.write">
80+
<user-settings data-targets="user-list.users">
81+
<input type="checkbox" data-targets="team-members.reads user-settings.reads">
82+
<input type="checkbox" data-targets="team-members.writes user-settings.writes">
8383
</user-settings>
8484
</user-list>
8585
</team-members>
@@ -112,6 +112,15 @@ class UserListElement extends HTMLElement {
112112
}
113113
```
114114

115+
### Target Vs Targets
116+
117+
To clarify the difference between `@target` and `@targets` here is a handy table:
118+
119+
| Decorator | Equivalent Native Method | Selector | Returns |
120+
|:-----------|:-------------------------|:-------------------|:-----------------|
121+
| `@target` | `querySelector` | `data-target="*"` | `Element` |
122+
| `@targets` | `querySelectorAll` | `data-targets="*"` | `Array<Element>` |
123+
115124
### What about without Decorators?
116125

117126
If you're using decorators, then the `@target` and `@targets` decorators will turn the decorated properties into getters.

package-lock.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@github/catalyst",
3-
"version": "0.1.0",
3+
"version": "0.3.0",
44
"description": "Helpers for creating HTML Elements as Controllers",
55
"homepage": "https://github.github.io/catalyst",
66
"bugs": {

src/findtarget.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ export function findTarget(controller: HTMLElement, name: string): Element | und
2020
export function findTargets(controller: HTMLElement, name: string): Element[] {
2121
const tag = controller.tagName.toLowerCase()
2222
const targets = []
23-
for (const el of controller.querySelectorAll(`[data-target~="${tag}.${name}"]`)) {
23+
for (const el of controller.querySelectorAll(`[data-targets~="${tag}.${name}"]`)) {
2424
if (el.closest(tag) === controller) targets.push(el)
2525
}
2626
return targets

test/findtarget.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ describe('findTargets', () => {
8787
chai.spy.on(instance, 'querySelectorAll', () => [])
8888
findTargets(instance, 'foo')
8989
expect(instance.querySelectorAll).to.have.been.called.once.with.exactly(
90-
'[data-target~="find-target-test-element.foo"]'
90+
'[data-targets~="find-target-test-element.foo"]'
9191
)
9292
})
9393

0 commit comments

Comments
 (0)