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.

Sign up for GitHub

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

Correction of the work with comments and general improvements #163

Merged
merged 13 commits into from
Oct 29, 2024
Merged

Correction of the work with comments and general improvements #163

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
54db511
Changed the if to switch construction in the Rules method.
Ulyanov-programmer Oct 14, 2024
19dcbdb
The duplicate code has been removed.
Ulyanov-programmer Oct 14, 2024
87fa306
Simplified the atruleNames function
Ulyanov-programmer Oct 14, 2024
e696ba6
Minor changes.
Ulyanov-programmer Oct 14, 2024
f40dfa6
Added a fix for not deleting comments
Ulyanov-programmer Oct 15, 2024
107d65d
Changing the tests
Ulyanov-programmer Oct 15, 2024
97f9fd0
Added code for processing comments in the rules.
Ulyanov-programmer Oct 17, 2024
d1d52f5
Updated tests.
Ulyanov-programmer Oct 17, 2024
a921dd3
Added processing for new lines.
Ulyanov-programmer Oct 17, 2024
5e4865c
Updated tests, added the use of new lines
Ulyanov-programmer Oct 17, 2024
69ac157
The readability of the test has been improved.
Ulyanov-programmer Oct 17, 2024
3233ebc
Added a test to save the father's comment
Ulyanov-programmer Oct 29, 2024
bc46ace
Edit to correct the lint error.
Ulyanov-programmer Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 67 additions & 65 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@ function interpolateAmpInSelector(nodes, parent) {
*/
function mergeSelectors(parent, child) {
let merged = []
parent.selectors.forEach(sel => {
for (let sel of parent.selectors) {
let parentNode = parse(sel, parent)

child.selectors.forEach(selector => {
for (let selector of child.selectors) {
if (!selector) {
return
continue
}
let node = parse(selector, child)
let replaced = interpolateAmpInSelector(node, parentNode)
Expand All @@ -67,22 +67,17 @@ function mergeSelectors(parent, child) {
node.prepend(parentNode.clone({}))
}
merged.push(node.toString())
})
})
}
}
return merged
}

/**
* Move a child and its preceeding comment(s) to after "after"
* Move a child and its preceding comment(s) to after "after"
* ! It is necessary to clarify the comment
*/
function breakOut(child, after) {
let prev = child.prev()
after.after(child)
while (prev && prev.type === 'comment') {
let nextPrev = prev.prev()
after.after(prev)
prev = nextPrev
}
return child
}

Expand All @@ -104,14 +99,12 @@ function createFnAtruleChilds(bubble) {
children.push(child)
}
})
if (bubbling) {
if (children.length) {
let clone = rule.clone({ nodes: [] })
for (let child of children) {
clone.append(child)
}
atrule.prepend(clone)
if (bubbling && children.length) {
let clone = rule.clone({ nodes: [] })
for (let child of children) {
clone.append(child)
}
atrule.prepend(clone)
}
}
}
Expand All @@ -125,16 +118,23 @@ function pickDeclarations(selector, declarations, after) {
after.after(parent)
return parent
}
function pickAndClearDeclarations(ruleSelector, declarations, after, clear = true) {
if (!declarations.length) return [after, declarations]

function atruleNames(defaults, custom) {
let list = {}
for (let name of defaults) {
list[name] = true
after = pickDeclarations(ruleSelector, declarations, after)

if (clear) {
declarations = []
}
if (custom) {
for (let name of custom) {
list[name.replace(/^@/, '')] = true
}

return [after, declarations]
}

function atruleNames(defaults, custom = '') {
let names = defaults.concat(custom)
let list = {}
for (let name of names) {
list[name.replace(/^@/, '')] = true
}
return list
}
Expand Down Expand Up @@ -297,10 +297,10 @@ module.exports = (opts = {}) => {
postcssPlugin: 'postcss-nested',

RootExit(root) {
if (root[hasRootRule]) {
root.walkAtRules(rootRuleName, unwrapRootRule)
root[hasRootRule] = false
}
if (!root[hasRootRule]) return

root.walkAtRules(rootRuleName, unwrapRootRule)
root[hasRootRule] = false
},

Rule(rule) {
Expand All @@ -310,46 +310,48 @@ module.exports = (opts = {}) => {
let declarations = []

rule.each(child => {
if (child.type === 'rule') {
if (declarations.length) {
after = pickDeclarations(rule.selector, declarations, after)
declarations = []
}

copyDeclarations = true
unwrapped = true
child.selectors = mergeSelectors(rule, child)
after = breakOut(child, after)
} else if (child.type === 'atrule') {
if (declarations.length) {
after = pickDeclarations(rule.selector, declarations, after)
declarations = []
}
if (child.name === rootRuleName) {
unwrapped = true
atruleChilds(rule, child, true, child[rootRuleMergeSel])
after = breakOut(child, after)
} else if (bubble[child.name]) {
copyDeclarations = true
unwrapped = true
atruleChilds(rule, child, true)
after = breakOut(child, after)
} else if (unwrap[child.name]) {
switch (child.type) {
case 'rule':
[after, declarations] = pickAndClearDeclarations(rule.selector, declarations, after)

copyDeclarations = true
unwrapped = true
atruleChilds(rule, child, false)
child.selectors = mergeSelectors(rule, child)
after = breakOut(child, after)
} else if (copyDeclarations) {
declarations.push(child)
}
} else if (child.type === 'decl' && copyDeclarations) {
declarations.push(child)

break
case 'atrule':
[after, declarations] = pickAndClearDeclarations(rule.selector, declarations, after)

if (child.name === rootRuleName) {
unwrapped = true
atruleChilds(rule, child, true, child[rootRuleMergeSel])
after = breakOut(child, after)
} else if (bubble[child.name]) {
copyDeclarations = true
unwrapped = true
atruleChilds(rule, child, true)
after = breakOut(child, after)
} else if (unwrap[child.name]) {
copyDeclarations = true
unwrapped = true
atruleChilds(rule, child, false)
after = breakOut(child, after)
} else if (copyDeclarations) {
declarations.push(child)
}

break
case 'decl':
if (copyDeclarations) {
declarations.push(child)
}

break
}
})

if (declarations.length) {
after = pickDeclarations(rule.selector, declarations, after)
}
pickAndClearDeclarations(rule.selector, declarations, after, false)

if (unwrapped && preserveEmpty !== true) {
rule.raws.semicolon = true
Expand Down
8 changes: 6 additions & 2 deletions index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,17 +605,21 @@ test('clears empty selector after comma', () => {
})

test('moves comment with rule', () => {
run('a { /*B*/ /*B2*/ b {} }', '/*B*/ /*B2*/ a b {}')
run('a { /*B*/ /*B2*/ b {} }', 'a { /*B*/ /*B2*/ } a b {}')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a expected output?

.block {
  /* Element comment */
  .element {
  }
}

will not be moved too?

Copy link
Contributor Author

@Ulyanov-programmer Ulyanov-programmer Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand you.
Your code should be converted to:

.block {
  /* Element comment */
}
.block .element {
}

It was from this logic that I transformed the test.

UPD: The code that was previously in the test was wrong - comments should not have left the rules in which they were embedded.
I've changed the logic and tests so that they stay inside the rules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code should be converted to

Is it expected output?

Seems like very soon other developers will come and will open new issues asking to not move this kind of comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really think that the comments embedded in the rules should remain outside?
If a comment is embedded in a rule, it should remain in it. If a comment is not embedded in a rule, it must be outside of it. It's logical, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, the plugin originally assumed this logic - when I wrote a comment in the rule, it remained in it (see the original issue). But in one of the cases there was a mistake that I fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use the library to convert CSS to HTML. In order to set the text for the elements, you need to enter it somewhere.
At first, custom properties were the solution, but they have a problem - they do not allow you to process multi-line texts.
For the correct processing of such text, I decided to use comments with a special mark, like this:

p {
  /* text:
    Lorem ipsum ipsum lorem
    Выходила на берег Катюша
  */
   a {
    --attr-href: 'https://www.npmjs.com/package/posthtml';
    --attrs: 'target="_blank" rel="noopener noreferrer"';
  }
}

Assigning text from comments when it is outside tags is quite problematic and illogical - the text must be inside the tag anyway, moreover, based on some features of working with CSS, I have an option to write text before and after the tag:

p {
  /* text-before:
    Lorem ipsum ipsum lorem
    Выходила на берег Катюша
  */
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. The problem is that it is unique feature for mostly 1-2 people. I do not want to change behvaiour for everyone.

The suggestions:

  1. You write a plugins which will run before (to hide comment to Rule.props) and after postcss-nested (to restore them).
  2. We can add keepComment option to the postcss-nested but only if it will be very small to not reduce code maintaibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another option: most often (a matter of formatting) there is an empty space between the properties and the nested rule. If they want to describe a rule with a comment, the comment is "pressed" to the rule from above, leaving no space.

This can be used to separate comments into those that cannot be left inside the rule and those that can.

Example:

p {
  /* comment inside the rule, save as is */

  /* comment for the rule below, move with the rule */
  a {
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's use empty new line to detect it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll take care of it in the next few days and let you know when the result is.

})

test('moves comment with at-rule', () => {
run('a { /*B*/ @media { one: 1 } }', '/*B*/ @media {a { one: 1 } }')
run('a { /*B*/ @media { one: 1 } }', 'a { /*B*/ } @media {a { one: 1 } }')
})

test('moves comment with declaration', () => {
run('a { @media { /*B*/ one: 1 } }', '@media {a { /*B*/ one: 1 } }')
})

test('moves comment with declaration without properties', () => {
run('a { @media { /*B*/ } }', '@media {a { /*B*/ } }')
})

test('saves order of rules', () => {
run('.one { & .two {} & .tree {} }', '.one .two {} .one .tree {}')
})
Expand Down
Loading