-
Notifications
You must be signed in to change notification settings - Fork 29
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
Adding User Preferences #57
base: gh-pages
Are you sure you want to change the base?
Conversation
index.html
Outdated
} | ||
}, | ||
"user_preferences": { | ||
"color_scheme_dark": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nested one level deeper now, see https://github.com/WICG/manifest-incubations/blob/gh-pages/user-preferences-explainer.md for the latest version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did see that and wanted to discuss: Would the manifest override construct hold up if there’s another level of nesting? I think the property > context = overrides makes sense, but in the revised user prefs model, it’s more like property > property > value = overrides. Feels inconsistent. In my view of these two features, having a single context to map to makes more sense.
Couldn’t the same thing be achieved by making the context "color_scheme_VALUE" (as we had it and how I used it) or "color_scheme: value" (or something similar)? What does the additional nesting give us in exchange for the cognitive overhead of having to manage more curly braces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this kind of nesting was strongly recommended by the TAG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the extra nesting was wanted by TAG.
I wonder if we need to define this as property > context > overrides? Could we just define the overrides part? Maybe we will also want to use this in future fields that don't fit this nesting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love a pointer to that TAG conversation (just for backstory). I thought on it a bit more and I think I have reasoned it out. user_preferences
has its own set of properties and those properties take a manifest override object. So the structure ends up being more like property > sub-property > context = overrides. It still feels unnecessarily complex (on another thread @marcoscaceres was actually pushing for flatter Manifest structures), but I can make it work from a spec standpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is TAG review here: w3ctag/design-reviews#696
I don't have a strong preference as to which structure we use, as long as the TAG reviewers are on board.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that. I'm good with the adjusted structure and I think we just define them (e.g., color_scheme
) as sub-properties whose values are a manifest override object. That does give us the extensibility for contrast, etc. too. Thanks for giving me the space to think through it a little more.
Can you update my conflict example accordingly or do you want me to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for thinking that through, that sounds good to me.
Yep, I can update that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an ex-TAG member, I'm not sure the TAG's opinions has more weight than anyone else's. Their guidance is always valuable and welcome, but we are free to explore a range of solutions that are best for developers.
Co-authored-by: Thomas Steiner <[email protected]>
Co-authored-by: Louise Brett <[email protected]>
on which user preferences are set. It has the following members: | ||
</p> | ||
<ul> | ||
<li>[=user_preferences/color_scheme=] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it as a note for now - happy to instead add it to the list if you prefer. I guess it'll need its own section if I add it to the list. Would we just want less
and more
for contrast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dig the note.
<p> | ||
The `user_preferences` member of the [=application manifest=] is an | ||
object that can be used to override values of manfiest members depending | ||
on which user preferences are set. It has the following members: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For:
"on which user preferences are set"
We should check the language that CSS uses for this, as it's not clear what "user preferences" means.
</ul> | ||
<p class="note"> | ||
This list of members is expected to expand in the future to include | ||
other <a data-cite="mediaqueries-5/#mf-user-preferences">user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to avoid hard links (data-cite="x#y"
) to other specs... better to file a bug on the CSS spec to export the terms (bonus points for sending a PR to export the terms needed!🏆)
Also, please avoid link to complete sections in the CSS spec. If we need to share concepts, we should try to specify that.
extension-point=] in [=processing a manifest=]: | ||
</p> | ||
<ol class="algorithm"> | ||
<li>If |json|["user_preferences"] does not [=map/exist=], return. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should happen in the main processing steps.
Sorry, I think this is too complicated and quickly becomes unmanageable 😔 The translations and color scheme example hits at the problem. I'd like to explore something like: {
"user_preferences": {
"color-scheme: dark": {},
"color-scheme: dark, language: es": {},
"color-scheme: dark, language: fr": {}
}
} This could solve for the specificity problem. It flattens the structure into something manageable, and is quite easy to read and parse. (If we can solve the above using CSS syntax, even better... then we don't need to create a new micro-syntax... however, it might not be unavoidable because we need to translate these values into things OSs can use, which is are not a CSS environment.) |
Combining this with translations is a good idea. I think we should avoid putting so much information into the keys though. What about something like this: "user_preferences": [
{
"color_scheme": "dark",
"language": "es",
"icons": [],
"theme_color": ""
}
] This is very similar to one of your previous proposals in the original thread. Or similar to @aarongustafson's proposal further down in the thread: "user_preferences": [
{
"context": {
"color_scheme": "dark",
"language": "es"
},
"overrides": {}
}
] |
</li> | ||
</ol> | ||
</li> | ||
<li>Let |overriden_manifest| be the result of creating a new [=ordered map=] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcoscaceres I’m not sure the best way to express this, but in JavaScript it would be merging the objects using the spread operator:
let overriden_manifest = {
…manifest,
…allowed_overrides
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See map/iterate ... something like:
[=map/For each=] |key| → |value| of |overrides|, set |manifest|[key] to |value|.
Should do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think that handles nested objects as neatly as spread, but I should be able to recursively call it if the value is an object.
</p> | ||
<p> | ||
To <dfn>apply a manifest override object</dfn>, given [=ordered_map=] | ||
|overrides:json|, [=ordered map=] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|overrides:json|, [=ordered map=] | |
|overrides:ordered map|, [=ordered map=] |
I’m not averse to the idea of combining translations into the same block (though we may want to rethink "user_preferences" as a property in that instance), but I feel like there are a coup[le of things at play from a developer ergonomics, teachability & manageability standpoint that we should be considering as well, based on how these are most likely to be used. We should establish a set of guiding principles for ourselves in evaluating potential approaches. I have some suggestions that I’m open to discussing/refining:
Given all of this, I think I lean towards the approach @loubrett shared:
To map this to what @marcoscaceres was looking at earlier, to avoid the specificity problem, you’d end up with something like "user_preferences_or_other_name_tbd": [
{
"context": {
"color_scheme": "dark"
},
"overrides": { }
},
{
"context": {
"color_scheme": "dark",
"lang": "es"
},
"overrides": { }
},
{
"context": {
"color_scheme": "dark",
"lang": "fr"
},
"overrides": { }
}
] This avoids the microsyntax problem and allows for expansion of the context block. We would not need to deal with specificity as we can rely on known JSON/JavaScript behavior where redefining a property updates that value of that property if the context applies. In other words any subsequent override that applies in the context would trump earlier ones. By way of example, if each of the The last thing we’d need to work out is whether redefining complex objects (e.g., updated_manifest = { …original_manifest, …overrides }; |
Right, but it comes with the verbosity of having to add and ever growing list of new members: If we just say, Again, consider: {
"name": "game",
"icons": [{}, {}],
"user_preferences": {
"prefers-color-scheme: dark and prefers-language: es": {
"icons": [{}, {}],
"name": "juego"
},
"prefers-color-scheme: dark and prefers-language: ja": {
"icons": [{}, {}],
"name": "ゲーム"
}
}
} So, given the criteria:
This holds. The MQ and the overrides don't require any nesting or new members. They only new thing they need to know is Again, compare the MQ solution to the Further, is solves for
This holds. Only overrides whatever is matched.
This holds. The MQ makes the context clear. And the members listed are the override.
This holds (or goes away), as there is no restriction. So it's always obvious. |
So are you thinking that every property would be open to updating, including Are you open to atomic updates? For example, swapping only the |
As a starting point, yes. We do have a notion of "security sensitive members", but we would need to consider them on a case-by-case basis. For example, it might make sense to change the What comes out after applying the user preferences is always a flat manifest, so it might be fine to just replace all things (including
No. I think it adds too much complexity. I know the tradeoff is more redundancy, but I'd prefer we apply the "keep it simple" principle. |
I'd like to avoid parsing the keys. I'd be happy with the condition as a string but it should be a value since it needs parsing. Maybe something like: "user_preferences": [
{
"context": "prefers-color-scheme: dark and prefers-language: es",
"overrides": { }
}
] But I'm not sold on using media query syntax. I think it would be simpler to have a set of values we support for user preferences (eg color_scheme_dark, color_scheme_light, etc.) which can be used in combination with language codes for translations.
I also lean towards allowing just overriding specifc parts of the object here but I can understand wanting to avoid that. |
@loubrett, wrote:
Let's game this out a bit as we have shared goals. I might be missing something obvious so I'm going to ask a bunch of silly questions. Please bear with me. 🙏
"user_preferences": [
{
"context": "prefers-color-scheme: dark and prefers-language: es",
"overrides": { }
}
] Questions:
i.e., it's the same as: "user_preferences": {
// context
"(prefers-color-scheme: dark) and (prefers-language: es)": {
// overrides
}
}
I kinda started at the same place and have bounced between MQ syntax and us specifying something new. The problem is that, when it's all put together, we basically end up at the same place: some kind of matching/query language that uses keywords/things that are already in CSS MQ. Consider the problem with "prefers dark mode" AND "prefers Spanish". To simultaneously express that with just JSON structures gets a little crazy with all the nesting. And coming up with our own thing is just reinventing the wheel, no? So, my question is, if we have MQs already (and we have parsers in the browsers), and they both end up functionally at the same place, why not just use MQs? |
Playing around a bit more... a "user_preferences" member probably doesn't make sense in the context of media queries. So, just an
|
I do like the simplicity of your syntax, but I feel that dictionary keys are not the right place to represent this data. I expect keys to be simple strings, not something that needs to be parsed.
I'm not expecting more keys here, I was just trying to think of a way to turn the context string from a key into a value.
It will be more difficult to implement if we use MQs. @mgiuca brought up some issues on the original thread.
I agree |
I like where things are heading in terms of simplifying the structure & approach. I am not against using the same structure as MQs and I think it would be up to the UA to decide how to handle things (i.e., whether they actually involve the MQ parser in determining applicability). Regardless we do need to be explicit about the fact that UAs will need to throw away overrides whose context includes unrecognized/unsupported strings/values.
Generally, I agree. I think where this really approach breaks down is redefining icons for The mechanism for merging objects like this should be (I would think) easy to implement. To show how this could work, I put together this demo: https://codepen.io/aarongustafson/pen/eYMmrro. I’m sure the merge code could be optimized, it’s just a quick & dirty example. More and more features that are on the horizon are complex JSON objects. Having to redefine each one as a whole is going to get quite unwieldly. Consider the following relatively simple example:
Following the "simple" approach, I would need to have the following
Compare this to supporting the more surgical approach: "overrides": {
"(prefers-color-scheme: dark)": {
"background_color": "#5b5b5b",
"icons": [
{ "src": "/i/icon-dark.svg" },
{ "src": "/i/icon-reverse.png" },
{ "src": "/i/notification-icon-reverse.png" }
]
},
"(prefers-language: no)": {
"description": "Aaron Gustafsons hjem og arbeid på nettet.",
"shortcuts": [
{ "name": "Notisbok" },
{ "name": "Snakker" },
{ "name": "Publikasjoner" },
{ "name": "Intervjuer" }
]
}
} Now I’m not against allowing folks to define the whole complex object if they want to, but the more surgical approach is far cleaner and truly simpler for developers. |
Preview | Diff