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.

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Ulyanov-programmer
Copy link

@Ulyanov-programmer Ulyanov-programmer commented Oct 15, 2024

List of changes:

  • Fixed a bug where rules with comments without properties could lose comments embedded in them.
  • The code that was duplicated several times was deleted and moved to a new function.
  • General code improvements: foreach has been replaced with for, and some methods have been optimized.

All tests were passed correctly.

@@ -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
Author

@Ulyanov-programmer Ulyanov-programmer Oct 15, 2024

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
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
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
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
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
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants