[#669] Declarative partial update use case - #958
Conversation
tunetheweb
left a comment
There was a problem hiding this comment.
See comment here regarding whether we should even add this yet:
But maybe it's OK to add this simple use case? But I have a few comments on that first.
Co-authored-by: Barry Pollard <barrypollard@google.com>
|
@jamesnw is this ready for another review? |
…dance-src into 669-declarative-partial-update
@rviscomi Yes, but the blocker is now this question, on whether it is too early to add this to MWG. |
Ah ok, I missed that. I'll reply there. Thanks! |
…dance-src into 669-declarative-partial-update
|
This now is a higher level guide, as recommended here. |
Co-authored-by: Jeremy Wagner <malchata@users.noreply.github.com>
…dance-src into 669-declarative-partial-update
tunetheweb
left a comment
There was a problem hiding this comment.
Apologies for the delay here and we'll wait for web-platform-dx/web-features#4318 but have some comments in the meantime.
Co-authored-by: Barry Pollard <barrypollard@google.com>
…dance-src into 669-declarative-partial-update
tunetheweb
left a comment
There was a problem hiding this comment.
LGTM but since we've waited this long let's give it a few more days to see if we can resolve web-platform-dx/web-features#4318 as that might need some more changes here.
| {{ BASELINE_STATUS("html-setters") }} | ||
| {{ BASELINE_STATUS("html-streaming-setters") }} |
There was a problem hiding this comment.
You could prefix these with tmp- to silence the CI checks, if you did want to merge these changes now. We'll automatically catch when the next version of web-features includes the feature IDs and remove the prefix. But it's also fine to wait for the new IDs to propagate.
There was a problem hiding this comment.
That doesn't work in the {{ BASELINE_STATUS() }} macro- there's no check built in for the pending web features.
(It even fails if you comment out (<!-- {{ BASELINE_STATUS("tmp-html-setters") }} -->) the feature, although I guess that makes sense since macros can insert content into comments)
There was a problem hiding this comment.
Oh interesting, that sounds like a bug. Let me know if you want to merge this as-is and this is the only blocker, then I can look into a fix. Otherwise the easiest thing would be to wait until the feature ID is published.
Alternatively you could do something like this, and in the next web-features update PR, we'll get a CI check telling us to remove the tmp prefix and we can resolve this TODO while we're in there.
| {{ BASELINE_STATUS("html-setters") }} | |
| {{ BASELINE_STATUS("html-streaming-setters") }} | |
| <!-- TODO: Use macros | |
| BASELINE_STATUS("html-setters") | |
| BASELINE_STATUS("html-streaming-setters") | |
| --> |
There was a problem hiding this comment.
No rush to merge this. Especially as half of it isn’t available until 154 anyway. Though the fact you have to use polyfills for other browsers anyway means you could use it now.
Anyway let’s five it a little longer.
There was a problem hiding this comment.
This is released, so I've removed the tmp features. We'll need to wait until web-features gets updated to 3.39.0 on this repo.
…dance-src into 669-declarative-partial-update
|
@tunetheweb I added an example of |
Agreed. |
|
This is ready to merge. (This is initially written as a discipline guide, rather than a use case, without demo or expectation files.) |
A full use case here would require a server streaming HTML slowly, which seems out of scope for this project. Would it suffice to have the demo just show the fully loaded HTML, which would still demonstrate the template replacement? The demo.html here copied from the official example shows what I mean. The guide could mention the server needs but not include code for the server side.
I suspect this won't pass CI without a web feature.
Also, should this be in performance rather than UX?