Re: [w3c/webcomponents] Update Imperative-Shadow-DOM-Distribution-API.md (#866)

domenic commented on this pull request.

Exciting to see this being worked on! Some suggestions on how to make it better.

>  
-The straw-man proposal for Imperative Shadow DOM distribution API.
-The context is: [https://github.com/whatwg/html/issues/3534](https://github.com/whatwg/html/issues/3534)
+This is a strawman proposal for Imperative Shadow DOM distribution API.

```suggestion
This is a strawperson proposal for Imperative Shadow DOM distribution API.
```

>  
-The straw-man proposal for Imperative Shadow DOM distribution API.
-The context is: [https://github.com/whatwg/html/issues/3534](https://github.com/whatwg/html/issues/3534)
+This is a strawman proposal for Imperative Shadow DOM distribution API.
+For more context, please see [the WhatWG issue](https://github.com/whatwg/html/issues/3534).

```suggestion
For more context, please see [the WHATWG HTML Standard issue](https://github.com/whatwg/html/issues/3534).
```

>  
-Mixing declarative API and imperative API would be troublesome and can be the cause of confusions for web developers.
-We *can* invent complex rules, however, no one wants to remember complex rules. Also, supporting both in the same shadow tree would make a browser engine complex, which I don't want.
-
-Thus, we don't allow mixing declarative API and imperative API in the same shadow tree.
-Web developers have to show their *opt-in* to use imperative API for each shadow tree.
-
-A shadow root has an associated *slotting*. Web developers can set shadow root's *slotting* to *manual* by specifying it in attachShadow:
-
-```js
-const sr = attachShadow({ mode: ..., (optional) slotting: 'manual' })
+One of the drawbacks of Shadow DOM v1, when compared to Shadow DOM v0, is that web developers have to specify slot= attribute for every shadow host's children (except for elements for the *default* slot).
+ 
+#### Case 1:  Slot attributes is required in markup.
 ```

```suggestion
```html
```

>  ```
+<shadow-host>
+  <div slot=slot1></div>
+  <div slot=slot2></div>
+</shadow-host>
+ ```
+Some people would see this as a kind of *ugly* markup.
+Shadow DOM v1 can't explain how `<summary>/<details>` elements can be implemented on the top of the current Web Components technology stack, given that `<details>` element doesn't need slot= attribute.
+Blink has a special logic for some built-in elements to control node-to-slot mapping.
+ 
+#### Case 2: Can’t slot based on condition. 
+``` 

Style issue: Use `html` after the three backticks, and fix all the curly quotes here to be normal quotes.

> +  <div slot=slot2></div>
+</shadow-host>
+ ```
+Some people would see this as a kind of *ugly* markup.
+Shadow DOM v1 can't explain how `<summary>/<details>` elements can be implemented on the top of the current Web Components technology stack, given that `<details>` element doesn't need slot= attribute.
+Blink has a special logic for some built-in elements to control node-to-slot mapping.
+ 
+#### Case 2: Can’t slot based on condition. 
+``` 
+<shadow-host num-to-show=”2”>
+   <div slot=”slot1”></div>
+   <div slot=”slot1”></div>
+   <div slot=”slot1”></div>
+</shadow-host>
+ ```
+The second issue is that component creators can’t change the slotting behavior based on condition. A component that wants to display a subset of slottable based on a property can’t easily be done via declarative slotting.

It'd be good to explain the connection to the above markup. Something like "So, there is no way to implement the `num-to-show` attribute in the above example.

It'd be even better if the example were more realistic, e.g. using an attribute like "tab" instead of "num-to-show" and a tag name like "tab-panel" instead of "shadow-host".

>  
+An `HTMLSlotElement` gets a new API, called `assign()` (tentative name), and a new property, `manually-assigned-nodes`,

Properties in JavaScript are usually named in camel case, e.g. `slot.manuallyAssignedNodes`. Maybe you mean "associated value", instead of property?

> + ```
+Some people would see this as a kind of *ugly* markup.
+Shadow DOM v1 can't explain how `<summary>/<details>` elements can be implemented on the top of the current Web Components technology stack, given that `<details>` element doesn't need slot= attribute.
+Blink has a special logic for some built-in elements to control node-to-slot mapping.
+ 
+#### Case 2: Can’t slot based on condition. 
+``` 
+<shadow-host num-to-show=”2”>
+   <div slot=”slot1”></div>
+   <div slot=”slot1”></div>
+   <div slot=”slot1”></div>
+</shadow-host>
+ ```
+The second issue is that component creators can’t change the slotting behavior based on condition. A component that wants to display a subset of slottable based on a property can’t easily be done via declarative slotting.
+
+## Imperative Slotting API

This section contains no code examples, and jumps straight into a discussion of the spec mechanisms and internal ways in which something will be implemented. See https://github.com/w3ctag/w3ctag.github.io/blob/master/explainers.md for a bit more on how to write a good explainer, in particular by including example code showing the API solving problems---perhaps the two problems you've introduced in the beginning.

I would:

- Move the discussion of internal concepts and implementation details to an appendix near the end.
- Add some basic examples (not as long or detailed as the ones you have below) showing how this API solves the problem.

>  
-See also the Example 3 later.
+```js
+const sr = attachShadow({ mode: ..., (optional) slotAssignment: 'manual' });

This isn't valid JavaScript syntax, so it's kind of confusing. I suggest:

```suggestion
const sr = attachShadow({ mode: 'open', slotAssignment: 'manual' });
```

>  
-Note: The detail is explained later, however, it would be worth noting that *manually-assigned-nodes* is not used as
-[assigned nodes] as is. You can think that `slot.assign(sequence<Node> nodes)` tell the engine *candidate nodes* from where [assigned nodes] are constructed.
+Note: The detail is explained later, however, it would be worth noting that `manually-assigned-nodes` is not used as

Where is "later"? Can you link to it?

>  
-## ShadowRootInit
+### ShadowRootInit
 
 
 ``` webidl
 ShadowRootInit {

It would be good to rewrite this closer to valid Web IDL.

>  │   ├── slot1
 │   └── slot2
 ├── A
 └── B
-
 ```
 
 ``` javascript

The "== means ArrayEquals" is very confusing here since it means is isn't JavaScript code. It would be good to replace this all with `assert_array_equals`.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/webcomponents/pull/866#pullrequestreview-366695611