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

Add tests for ignoring properties case for ordering. Related to #78 #196

Merged
merged 6 commits into from
Oct 9, 2024

Conversation

Astrael1
Copy link
Contributor

@Astrael1 Astrael1 commented Oct 7, 2024

Hello :)
There are some issues with #78

  1. Shorthands
    I don't know how the plugin should behave with shorthands. I assumed that if user types

Border-width: 1px; Border-top-width: 2px;

they do it on purpose and they expect it to behave just like any other css properties. Do you agree with that?

  1. It needs postcss-sorting to have an option to ignore case. Shall I report that issue there?
  2. Do you have any more tests ideas? Are you willing to merge the tests without other changes?

@hudochenkov
Copy link
Owner

hudochenkov commented Oct 7, 2024

  1. Shorthands area already supported
    // OK if the first is shorthand for the second:
    if (isShorthand(firstPropData.unprefixedName, secondPropData.unprefixedName)) {
    return true;
    }
    // Not OK if the second is shorthand for the first:
    if (isShorthand(secondPropData.unprefixedName, firstPropData.unprefixedName)) {
    return false;
    }
    I don't expect anything new needed for this.
  2. Correct. First postcss-sorting need to ignore case, then we can add it to stylelint-order. Because postcss-sorting powering autofixing, it would be strange if logic for checking order in stylelint-order would be different than its fixing functionality.
  3. I've added few suggestions. Also need tests for properties-order.

@hudochenkov hudochenkov changed the title tests for #78 Add tests for ignoring properties case for ordering. Related to #78 Oct 7, 2024
Szymon Małolepszy added 3 commits October 8, 2024 09:15
Tests for properties-alphabetical-order and flat, grouped-flexible,
validate-options in properties-order
{
emptyLineBefore: 'always',
order: 'flexible',
properties: ['top', 'Top'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hudochenkov I need your decision on this. User may provide above properties. In this case they should be informed, that the plugin doesn't work like that.

@Astrael1
Copy link
Contributor Author

Astrael1 commented Oct 8, 2024

I added some tests. I haven't added tests in grouped-strict yet.

I'd like to know if it's fine. Are there too many tests or not enough etc?

rules/properties-alphabetical-order/tests/index.js Outdated Show resolved Hide resolved
rules/properties-order/tests/flat.js Outdated Show resolved Hide resolved
rules/properties-order/tests/flat.js Outdated Show resolved Hide resolved
rules/properties-order/tests/flat.js Outdated Show resolved Hide resolved
rules/properties-order/tests/grouped-flexible.js Outdated Show resolved Hide resolved
rules/properties-order/tests/validate-options.js Outdated Show resolved Hide resolved
@hudochenkov
Copy link
Owner

I haven't added tests in grouped-strict yet.

They are not needed.

Copy link
Owner

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Thank you!

@hudochenkov hudochenkov merged commit 2453a16 into hudochenkov:master Oct 9, 2024
3 checks passed
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