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

Correct evaluation of HTML custom properties and JSON support #1001

Merged
merged 13 commits into from
Nov 29, 2022

Conversation

brosua
Copy link
Contributor

@brosua brosua commented Jan 17, 2022

Description

The data attribute was mispelled. Also I added a logic to parse JSON, if multiple custom attributes have to be added.
It fixes #982.

Screenshots (if appropriate)

Types of changes

  • Chore (tooling change or documentation change)
  • Refactor (non-breaking change which maintains existing functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • My code follows the code style of this project.
  • I have added new tests for the bug I fixed/the new feature I added.
  • I have modified existing tests for the bug I fixed/the new feature I added.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@mtriff mtriff added the bugfix Pull request that fixes an existing bug label Jan 17, 2022
@mtriff
Copy link
Member

mtriff commented Jan 17, 2022

Thanks! This will need unit tests for the new function you added and some e2e tests for each input type. It looks like you need to run the linter as well.

@mtriff mtriff added the changes required Pull request requires changes before it can be merged label Jan 17, 2022
@mtriff
Copy link
Member

mtriff commented Mar 13, 2022

@brosua are you still interested in getting this merged?

@brosua
Copy link
Contributor Author

brosua commented Mar 14, 2022

Yes, I will try to update the MR this week

@brosua
Copy link
Contributor Author

brosua commented Mar 29, 2022

Hello @mtriff , you can check the updated MR.

@Lewke
Copy link

Lewke commented Apr 1, 2022

awaiting this, really need the functionality, thanks for fixing it :)

@reimax
Copy link

reimax commented May 3, 2022

awaiting this pr

Nugjii
Nugjii previously approved these changes Nov 2, 2022
@Nugjii
Copy link

Nugjii commented Nov 2, 2022

Awaiting this PR, please review and merge.

@mtriff
Copy link
Member

mtriff commented Nov 19, 2022

Hello all, sorry for the long delay in getting this merged. I'd like to get this into the next release as well!

Taking a look at the branch, it still needs an e2e test. I've been testing it out locally and it looks like JSON custom properties breaks search with Fuse.js (errors in the console). It looks like this might be fixed by upgrading dependencies, but I haven't had a chance to look into it further.

mtriff
mtriff previously approved these changes Nov 28, 2022
@mtriff mtriff removed the changes required Pull request requires changes before it can be merged label Nov 29, 2022
@mtriff mtriff changed the title [BUGFIX] Correct evaluation of custom properties [BUGFIX] Correct evaluation of HTML custom properties and JSON support Nov 29, 2022
@mtriff mtriff merged commit a7ed4d8 into Choices-js:master Nov 29, 2022
@mtriff
Copy link
Member

mtriff commented Nov 29, 2022

Thank you for the contribution @brosua!

@mtriff mtriff added the feature Pull request that adds new functionality label Nov 29, 2022
@mtriff mtriff changed the title [BUGFIX] Correct evaluation of HTML custom properties and JSON support Correct evaluation of HTML custom properties and JSON support Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Pull request that fixes an existing bug feature Pull request that adds new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use data-custom-properties
5 participants