-
Notifications
You must be signed in to change notification settings - Fork 63
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
Proposal: add $deprecated property #255
Conversation
✅ Deploy Preview for dtcg-tr ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
## More token properties TBC | ||
### Deprecated | ||
|
||
The optional **`$deprecated`** property is a boolean or string where tooks MAY specify deprecated tokens. A token MAY be marked deprecated in any of the following scenarios: |
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.
cc @c1rrus any nitpicking of language here would be greatly appreciated 🙏. Happy to rephrase this to match the existing language (or if you wanted to do a pass on it that would be fine, too!)
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.
Thank you. LGTM 👍
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.
Spotted a couple of teeny typos. But otherwise this looks good. Happy to approve once they get fixed.
9f24807
to
ef61320
Compare
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.
Nice one. Thanks for doing this @drwpow!
Thanks! Will give other editors a chance to review and leave feedback over the next week or 2. If there’s no objection, we’ll merge! But also, based on the other recent proposals, generally the editors express the same opinions found in the comments, so the fact it’s being proposed in the first place is usually positive 🙂 |
To confirm, there is neither a necessity nor a spec-defined way to confirm
or extract an alternate token suggestion inside the deprecated tag,
toolmakers can make their own assumptions on if and how to programmatically
determine it?
…On Fri, 1 Nov 2024 at 01:31, Kaelig Deloumeau-Prigent < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In technical-reports/format/groups.md
<#255 (comment)>
:
> + "Button": {
+ "$deprecated": "Please use {action.*} tokens instead.",
+ "Foreground": { "$value": "#202020", "$type": "color" },
+ "Background": { "$value": "#ffffff", "$type": "color" }
+ }
+}
+```
+
+</aside>
+
+In the context of a group, adding `$deprecated` will apply to all tokens within that group, unless a token explicitly sets a value of `false`. Any value provided by a token will override the group default.
+
+| Value | Explanation |
+| :------- | :---------------------------------------------------------- |
+| `true` | This token is deprecated (no explanation provided). |
+| `String` | This token is deprecated AND this is an explanation. |
Perhaps it would be useful to mention that tool makers MAY augment the
string when it contains aliases such as the one given as an example:
Please use {button.activeBorder} instead
—
Reply to this email directly, view it on GitHub
<#255 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEKS36AFBRW2TZODLTREIVLZ6KEBLAVCNFSM6AAAAABQR27HUKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMBYHE3TEMBVHE>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
@drwpow good to merge? |
Almost! We had been waiting to see if one group had feedback. Will ping them again and if not, will merge! |
3fd459b
to
c132b49
Compare
Correct. Updated the wording per @kaelig’s suggestion to describe this a bit more. By default we don’t have aliases in the examples anymore outside the section that describes “it’s up to tools to determine how they do/don’t handle parsing the |
We’ve waited for additional parties to give input / feedback but haven’t gotten any pushback or corrections. Merging! |
SHA: af7e799 Reason: push, by drwpow Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
This adds a new
$deprecated
property for both tokens and groups as suggested by @romainmenke.Reasoning
This PR basically accepts #118 as-proposed (the original proposal), while taking into account feedback from followup comments.
Pros
Cons
$deprecated
in the future would be a breaking changeAlternatives
$deprecated
/$deprecatedMessage
) isn’t a design pattern that currently exists in the spec. Pair properties can also be antipatterns as they introduce validation complexity (e.g. what if$deprecatedMessage
is set but$deprecated
isn’t—is that valid or invalid? and if valid, what behavior is inferred?)$deprecated.deprecated
/$deprecated.message
) wasn’t proposed here because it adds unnecessary boilerplate.$deprecated
. Or in areas where they potentially overlap, will be complementary rather than conflicting.Notes