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

Jump to bottom

Implement option to support adding spaces between CSS selectors #2029

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

Implement option to support adding spaces between CSS selectors #2029

wants to merge 9 commits into from

Conversation

kcamsanc
Copy link
Contributor

@kcamsanc kcamsanc commented Apr 10, 2022 •
edited by bitwiseman

Description

  • Source branch in your fork has meaningful name (not main)

Fixes Issue:
Fixes #1862 - Added a new option called selector-separator.
Added unit tests and updated the README/CLI options accordingly.

Before Merge Checklist

These items can be completed after PR is created.

(Check any items that are not applicable (NA) for this PR)

  • JavaScript implementation
  • Python implementation (NA if HTML beautifier)
  • Added Tests to data file(s)
  • Added command-line option(s) (NA if
  • README.md documents new feature/option(s)

@kcamsanc kcamsanc mentioned this pull request Apr 10, 2022
Open
bitwiseman
bitwiseman requested changes Apr 11, 2022
View reviewed changes
Copy link
Member

@bitwiseman bitwiseman left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for working on this!

You'll need to resolve conflicts.

Remove selector-separator-newline from the documented options and note it is deprecated in code comments.

I think I'd rather have the option be named selector-separator.
Default value is "default". If the value is set to null or "", the value will be treated as "default".

  • If set to "default" and selector-separator-newline is false, then add a space between selectors
  • If set to "default" and selector-separator-newline is true, then add a newline between selectors
  • If set to anything other than "default", then ignore selector-separator-newline and:
    • If set to "none", then remove all whitespace between selectors.
    • If set to "newline", then add a newline between selectors.
    • If set to "space", then add a space between selectors.
    • For any other value, throw error.

@kcamsanc
Copy link
Contributor Author

Sounds good, I'll address this soon.

If set to "default" and selector-separator-newline is false, then add a space between selectors
If set to "default" and selector-separator-newline is true, then add a space between selectors

To clarify, if selector-separator is set to "default", we want to add a space between selectors regardless of the value of selector-separator-newline?

@bitwiseman
Copy link
Member

bitwiseman commented Apr 12, 2022 •
edited

No, sorry. I've updated my comment. "default" is the same as current behavior.

@kcamsanc
Copy link
Contributor Author

Okay, just updated my changes based on your comments, including updating the README and unit tests. Let me know how it looks.

@bitwiseman
Copy link
Member

Looks like you have some conflicts to resolve before we can run the build.

@bitwiseman bitwiseman marked this pull request as draft April 21, 2022 17:04
@kcamsanc
Copy link
Contributor Author

Just resolved those conflicts👍🏼

@kcamsanc kcamsanc marked this pull request as ready for review April 21, 2022 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bitwiseman bitwiseman bitwiseman requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add option to add space between CSS selectors
2 participants
@kcamsanc @bitwiseman