-
-
Notifications
You must be signed in to change notification settings - Fork 928
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
Add "ignoreLonghands": []
to declaration-block-no-redundant-longhand-properties
#7232
Comments
text-decoration-width
fix support to declaration-block-no-redundant-longhand-properties
text-decoration-thickness
fix support to declaration-block-no-redundant-longhand-properties
Iv reverted to status: needs discussion because on a browser that doesn't support a {
text-decoration-thickness: 1px;
} will simply be ignored, but
will be dropped altogether because it will consider it invalid. tl;dr: if it's not configurable (pick between 3 or 4) it's not worth adding IMHO |
Hum, I think it's still worth to support a {
text-decoration-line: underline;
text-decoration-style: solid;
text-decoration-color: green;
text-decoration-thickness: 1px;
/* ↓↓↓ autofixed */
text-decoration: underline solid green 1px;
} At the same time, we'd like to keep the current behavior if a {
text-decoration-line: underline;
text-decoration-style: solid;
text-decoration-color: green;
/* ↓↓↓ autofixed */
text-decoration: underline solid green;
} Can we achieve this by the rule's implementation? @mattxwang |
That's possible, but as I have explained in the previous comment, it is not worth adding unless we have a new option that let's us ignore a {
text-decoration-line: underline;
text-decoration-style: solid;
text-decoration-color: green;
text-decoration-thickness: 1px;
} would autofix to a {
text-decoration: underline solid green;
text-decoration-thickness: 1px; // only this line is dropped if the browser doesn't support it
} i.e. the existing |
Currently, caniuse shows 93% coverage at the global: It may make sense to wait until this coverage increases more. We can revisit this issue when the coverage is enough. |
It wouldn't change my opinion: it requires a new option. |
@Mouvedia You mean a new option like |
@ybiquitous Should I create a new issue for that option? |
Make sense. Adding a new
No need. Let's continue the discussion here so that we can easily understand the context. So could you update the issue title and comment on a blueprint for the new option? |
text-decoration-thickness
fix support to declaration-block-no-redundant-longhand-properties
"ignoreLonghands": []
to declaration-block-no-redundant-longhand-properties
Ah! I think this issue slipped by me earlier. This is certainly doable, though it'll require quite a bit of refactoring to get it to work nicely with the existing approach. After v16, I am happy to work on this a bit (since, for better or worse, I've been heavily involved with this rule and its autofix). |
This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant. |
@mattxwang are you still planning to work on it? |
@Mouvedia go for it, I've been swamped with work lately and haven't had the chance to work on Stylelint :( |
Based on @jeddy3's 👍 I've labeled the issue as ready to implement. Please consider contributing if you have time. There are steps on how to add a new option in the Developer guide. |
This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant. |
@mattxwang are you still interested in implementing that enhancement? |
I don't think I'd be able to dedicate time to this until the summer (~June) after I'm done teaching. If you'd like to take it on, go for it! (would you still want me to draft up an issue)? |
When you will have time. |
use cases
text-decoration
background
rationale
If a browser doesn't support the new arity for a given shorthand, it will drop it altogether.
For example if we were to introduce an enhancement for the current fixer of
text-decoration
by adding support totext-decoration-thickness
, for a lot of users of the rule it would introduce a hard to detect regression.They would have to rely on
"ignoreShorthands": ["text-decoration"]
which is arguably a downgrade compared to the status quo. Hence a new option is required to be more granular and cross-browser.text-decoration
rationale for new fix
#6580 (comment)
#7060
without option
becomes
with
"ignoreLonghands": ["text-decoration-thickness"]
becomes
references
https://bugs.webkit.org/show_bug.cgi?id=190774
https://drafts.csswg.org/css-text-decor-4/#text-decoration-property
implementation details
Firefox 69 supports this feature (preffed off) under the name
text-decoration-width
, behind the pref layout.css.text-decoration-width.enabled.text-decoration-skip
is not part of the shorthand but will have to be eventually added in a separate PR.browser support
https://caniuse.com/?search=text-decoration-thickness
background
rationale
https://caniuse.com/background-img-opts
example
becomes
The text was updated successfully, but these errors were encountered: