New: Replace :root selectors #168
Conversation
|
Looking for help in naming of rule code and warning message. |
src/polymer/root-selector-to-html.ts
Outdated
| code: this.code, | ||
| severity: Severity.WARNING, sourceRange, | ||
| message: stripWhitespace(` | ||
| Root should no longer be used |
There was a problem hiding this comment.
😆 Yeah... I really had no idea what to make this message
src/polymer/root-selector-to-html.ts
Outdated
| message: stripWhitespace(` | ||
| Root should no longer be used | ||
| `), | ||
| fix: [{range: sourceRange, replacementText}] |
There was a problem hiding this comment.
We need to replace just the :root part, so I think we'll want to get the replacementText via a regex replace of :root with the passed in replacementText
There was a problem hiding this comment.
I'm not sure what you mean on this
There was a problem hiding this comment.
How does this handle a selector like :root foo?
I think it would turn it into either html or :host > * but (I think) we want it to emit html foo or :host > * foo
src/polymer/root-selector-to-html.ts
Outdated
| ...this.generateWarnings( | ||
| document, isCustomStyle, 'html'), // Check custom styles | ||
| ...this.generateWarnings( | ||
| document, isElementStyle, ':host > *'), // Check element styles |
There was a problem hiding this comment.
If it's a dom-module without an element then it might be a style which is intended for inclusion with include, in which case we want to convert it to both html and :host. See, e.g. https://github.com/PolymerElements/paper-styles/blob/master/element-styles/paper-material-styles.html#L37
If there's other parts of the selector (does that make sense in this context? I'm not sure), we also need to preserve them, so that :root foo becomes :host > * foo, html foo
There was a problem hiding this comment.
hmmmm good call on the dom-module part, I will see if I can get that fixed as well.
As far as the addition parts to the selector, I'm not sure if it makes sense or not. I have only ever seen it in the context of :root { https://www.polymer-project.org/2.0/docs/upgrade#replace-root-selectors
There was a problem hiding this comment.
In that case, let's not fix except in the very simple case of just :root
stramel
force-pushed
the
ms/convert-root-selector
branch
from
76e2bc3
to
d5ec56a
Compare
|
Working on handling style modules |
…-linter into ms/convert-root-selector
|
@rictic This should be all good for review again |
src/polymer/root-selector-to-html.ts
Outdated
| @@ -123,14 +123,20 @@ class RootSelectorToHtml extends Rule { | |||
| const sourceRange = containingDoc.sourceRangeForShadyRange( | |||
| {start, end: start + match[0].length}); | |||
|
|
|||
| // Only fix plain `:root` selectors | |||
| let fix: Replacement[] = []; | |||
There was a problem hiding this comment.
I think this should be instead:
let fix: undefined|Replacement[] = undefined;
if (node.selector.match(/^:root$/) !== null) {
fix = [{range: sourceRange, replacementText}];
}Otherwise I think we tell the user that the warning could be fixed / was fixed even when we can't.
There was a problem hiding this comment.
Okay, for some reason, I thought I saw something like this elsewhere. I will make that change.
|
Arg github, don't hide that comment… |
|
-.- Fixing |
|
Great work! |
Relates to #142
/cc @rictic
This change is