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

allow extra spaces in headerCssClass #960

Merged
merged 2 commits into from
Jan 2, 2024

Conversation

jesenko
Copy link
Contributor

@jesenko jesenko commented Dec 31, 2023

Extra spaces in class dom attribute are allowed in html (e.g. class="some-class other-class "). SlickGrid should probably also behave the same way (or at least fail with some meaningfull validation error).

This commit just ignores all extra spaces in headerCssClass (between individual classes or leading / trailing).

I did not add E2E test for this, seems a bit overkill to add example html file for testing such minor case...
The error can be reproduced however by e.g. adding some space at the beginning / end of headerCssClass in examples/example-web-component-pubsub-esm.html:193

Extra spaces in `class` dom attribute are allowed in html (e.g. `class="some-class  other-class "`). SlickGrid should probably also behave the same way (or at least fail with some meaningfull validation error).

This commit just ignores all extra spaces in headerCssClass (between individual classes or leading / trailing).
@ghiscoding
Copy link
Collaborator

ghiscoding commented Dec 31, 2023

it seems like developers looking for trouble lol... I did some perf tests and this seems faster than trim() + regex, so I guess that's ok even though it's less readable. Note that there's like 15-20 use of these things across the project, just fixing 1 probably isn't good enough, I would suggest to add a simple function in the slick.core Utils and reuse it everywhere needed

// slick.core.ts Utils
function wordSplit(s = ''): string[] {
    return s.split(' ').filter(cls => cls);
}

then use it everywhere

header.classList.add(...Utils.wordSplit(classname));

there's a few .split(' '), just do a solution search, however not all of them require the fix, some of them are used just for reading from existing DOM elements which don't require any fixes

@ghiscoding
Copy link
Collaborator

ghiscoding commented Jan 2, 2024

@jesenko Thanks the PR is looking good, but I just find the method name to be a little long, could we simply use classNameToList instead?

… list

Utils.classNameToClassList method is introduced to return list of css classes
out of single space-separated className string.
@jesenko jesenko force-pushed the allow-spaces-header-css-class branch from edf87ab to a89f0bb Compare January 2, 2024 19:28
@jesenko
Copy link
Contributor Author

jesenko commented Jan 2, 2024

@ghiscoding renamed method to classNameToList, thank you for suggestion — I did not like the long name either, but was unable to come up with clear and concise naming :)

@ghiscoding ghiscoding merged commit 4beb2a5 into 6pac:master Jan 2, 2024
2 checks passed
@ghiscoding
Copy link
Collaborator

Shorter is better and we stayed concise so that's looking good now. Thanks for the contribution :)

@ghiscoding ghiscoding changed the title Allow extra spaces in headerCssClass fix: allow extra spaces in headerCssClass Jan 2, 2024
@ghiscoding
Copy link
Collaborator

ghiscoding commented Jan 2, 2024

ohh I just noticed that your PR didn't include fix or feat, please make sure to include that in your PRs next time so that the changelog can automatically detect it and assign to the proper group in the changelog, in the case of your PR it won't detect anything. This automation works when using Conventional Commits which is what most projects use nowadays

I had to do git commit --amend to manually fix your PR subject

@ghiscoding ghiscoding changed the title fix: allow extra spaces in headerCssClass allow extra spaces in headerCssClass Jan 2, 2024
ghiscoding pushed a commit that referenced this pull request Jan 2, 2024
* fix: allow extra spaces in headerCssClass and other cssClass

Extra spaces in `class` dom attribute are allowed in html (e.g. `class="some-class  other-class "`). SlickGrid should probably also behave the same way (or at least fail with some meaningfull validation error).

This commit just ignores all extra spaces in headerCssClass (between individual classes or leading / trailing).
@ghiscoding
Copy link
Collaborator

Shipped with v5.7.1 release, thank you

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