Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explainer feedback #1

Closed
yoavweiss opened this issue May 3, 2021 · 4 comments
Closed

Explainer feedback #1

yoavweiss opened this issue May 3, 2021 · 4 comments

Comments

@yoavweiss
Copy link
Collaborator

Hey @tomayac! Thanks for working on this! This seems like an important feature that would enable sites to optimize their CSS without jarring user-visible effects.

A few comments in no particular order:

  • It's better to split prefers-color-scheme and prefers-reduced-motion to 2 separate headers. That enable independent variance of these 2 values (from a Vary header perspective), and would match the decision we made with other Client Hints.
    • You probably also want to prefix these hints, to indicate they are browser sent, similar to other client hints
  • The "FART" acronym seems like a stretch to the concept of acronyms, but 🤷
  • I'd avoid meta notes 2 and 3, as I suspect they'd just confuse folks. The CH RFC just defines the infrastructure, and I don't know if many people remember the distant past where this wasn't the case. Same for Accept-CH-Lifetime
  • RE meta-note 4, all hints can be critical hints. It's the developer's decision whether this is something they want to configure their HTTP server to emit.

I suspect the first point will trigger a bunch of other changes to both explainer and spec. Happy to review if helpful! :)

@tomayac
Copy link
Collaborator

tomayac commented May 3, 2021

Hey @tomayac! Thanks for working on this! This seems like an important feature that would enable sites to optimize their CSS without jarring user-visible effects.

Thank you for taking the time to review this!

  • It's better to split prefers-color-scheme and prefers-reduced-motion to 2 separate headers. That enable independent variance of these 2 values (from a Vary header perspective), and would match the decision we made with other Client Hints.

For background, one motivation for using a generic "user preferences" header was that the list of such preferences is growing. Now already we have prefers-reduced-motion, prefers-reduced-transparency,prefers-contrast,prefers-color-scheme, forced-colors, and prefers-reduced-data. But I do see the Vary argument, so will split.

  • You probably also want to prefix these hints, to indicate they are browser sent, similar to other client hints

Any recommended prefix? UA-PREFERS-COLOR-SCHEME maybe?

  • The "FART" acronym seems like a stretch to the concept of acronyms, but 🤷

I saw it in the CSS Tricks article and was looking for an excuse to use it. If it's too tongue-in-cheek for a spec text, I'm more than happy to remove it.

  • I'd avoid meta notes 2 and 3, as I suspect they'd just confuse folks. The CH RFC just defines the infrastructure, and I don't know if many people remember the distant past where this wasn't the case. Same for Accept-CH-Lifetime

It's mostly me trying to prove that I did my due diligence. Happy to remove.

  • RE meta-note 4, all hints can be critical hints. It's the developer's decision whether this is something they want to configure their HTTP server to emit.

Fully aware, this was just me trying to somehow convey that most users would probably want to use this in "critical" mode. Or does this not need stating?

I suspect the first point will trigger a bunch of other changes to both explainer and spec. Happy to review if helpful! :)

I'd definitely ask you for a PTAL once I have refactored. Waiting for the answers to my questions raised now before I tackle this. Thanks (in advance) for your offer to help and the review!

@yoavweiss
Copy link
Collaborator Author

Any recommended prefix? UA-PREFERS-COLOR-SCHEME maybe?

Sec-CH-Prefers-*, most probably. The Sec- part helps ensure that the header was added by the UA, and not by JS code. There was also a discussion about it providing extra CORS safety, but for the moment it's unclear it will.

@yoavweiss
Copy link
Collaborator Author

If it's too tongue-in-cheek for a spec text, I'm more than happy to remove it.

It's fine to leave it in :)

Fully aware, this was just me trying to somehow convey that most users would probably want to use this in "critical" mode. Or does this not need stating?

It doesn't need stating, but can be, certainly in the explainer. You could clarify it by saying that developers are likely to use this as a critical hint.

@tomayac
Copy link
Collaborator

tomayac commented May 7, 2021

@yoavweiss, PTAL at the updated spec draft and the updated Explainer. I think this closes the present Issue. @beaufortfrancois, since you have expressed interest in implementing this (thanks!!!), I also invite you to take a look.

@tomayac tomayac closed this as completed May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants