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

Fix whitespace bug in x-bind:class class list #437

Merged
merged 3 commits into from
May 8, 2020

Conversation

felixsc
Copy link
Contributor

@felixsc felixsc commented Apr 29, 2020

Fixes #436

When the class list declaration in a x-bind:class is ended with a whitespace or contains multiple consecutive whitespaces an error is thrown.

This PR filters the class list after it is split and removes the empty classes.

Copy link
Contributor

@HugoDF HugoDF left a comment

Choose a reason for hiding this comment

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

Nice one, is there a reason the bundle changed so much? Looks to me like the output should only change as much as your code changes

} else {
classNames.split(' ').forEach(className => el.classList.remove(className))
classNames.split(' ').filter(Boolean).forEach(className => el.classList.remove(className))
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have a getClassNames util/helper that does the split + filter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, makes sense. It's repeated three times in this PR

@MuzafferDede
Copy link
Contributor

@HugoDF Yeah, There are some changes mixed up with the PR. Build files has some ghost changes.

@felixsc
Copy link
Contributor Author

felixsc commented Apr 30, 2020

Yeah, it felt a bit odd to me as well.

I didn't do anything besides installing the dependencies with
yarn
and building it after writing the changes with
npm run build
Any idea what might have caused it? Should I've done it differently?

@SimoTod
Copy link
Collaborator

SimoTod commented Apr 30, 2020

Weirdly, it seems like yarn install didn't use the package-lock and pulled a new version of core-js (it should have used this one https://github.com/felixsc/alpine/blob/master/package-lock.json#L2699).

Would you mind deleting the node_modules folder, using npm install and committing the dist folder again please? Thanks

The dependency should probably be updated but not in the scope of this PR. Thanks for your fix, it looks good.

@SimoTod
Copy link
Collaborator

SimoTod commented Apr 30, 2020

Right, Yarn doesn't use package-lock unless you run yarn import first.

As of Yarn 1.7.0, you can import your package-lock.json state, generated by npm to Yarn, by using yarn import.

Mistery solved.

@felixsc
Copy link
Contributor Author

felixsc commented Apr 30, 2020

Aha, didn't know that. TIL
Thanks for sharing

@felixsc felixsc closed this Apr 30, 2020
@felixsc felixsc reopened this Apr 30, 2020
@@ -420,3 +420,14 @@ test('setSelectionRange is not called for inapplicable input types', async () =>
expect(document.querySelector('input').value).toEqual('baz')
})
})

test('unnecessary whitespaces in class list are ignored', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to have tests for when it's using array syntax or string syntax?

@calebporzio
Copy link
Collaborator

Good fix! Thanks @felixsc!

@calebporzio calebporzio merged commit e32bea0 into alpinejs:master May 8, 2020
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.

DOMException for trailing whitespace in x-bind:class
5 participants