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

Schema class strings fail if using $reset as the first item due to schema parser. #61

Closed
andrew-boyd opened this issue Feb 7, 2022 · 7 comments
Assignees
Labels
🪄 enhancement New feature or request 💡 needs ideas Open to new suggestions and ideas 🔵 priority-3 3. Lower priority issue 🚀 release-ready Feature or fix is complete and on an upcoming release branch
Milestone

Comments

@andrew-boyd
Copy link
Member

andrew-boyd commented Feb 7, 2022

When supplying custom classes and using the $reset token, the schema attempts to parse the string resulting in the default classes persisting and the custom classes not being applied.

We probably need a different token for $reset unless it's acceptable to prevent -class schema items from being dynamically processed.

FormKit Vue Playground link:
https://formkit.link/5e27eb29b8894ccfb4431fe63238b92c

Update — CURRENT SOLUTION:

Prefix your $reset calls in schema with raw per the docs.
https://formkit.link/0f8d83e4f7b51791eef9aa544383fa81

@andrew-boyd andrew-boyd added the 🐛 bug Verified bug by team label Feb 7, 2022
@justin-schroeder
Copy link
Member

Yeah I dont think we should not process the -class prop — too many valid use cases would be impossible. Instead I think we might have 3 options:

  1. Make an exception for $reset, don’t treat it like a variable
  2. Educate people to use the __raw__inputClass prefix
  3. Come up with a new token other than the dollar-prefixed $reset

@andrew-boyd
Copy link
Member Author

Honestly, 3 seems like the most sensible thing, and now's the time to do it. some options:

%reset
_reset
!reset
@reset <-- this one feels sass / tailwind like.

@andrew-boyd andrew-boyd self-assigned this Feb 9, 2022
@justin-schroeder
Copy link
Member

One small sticking point with those is only _reset is a valid quoted object key in JS:

{
  $reset: true, // ✅ valid
  _reset: true,  // ✅ valid
  !reset: true,  // 🚫 not valid
  %reset: true,  // 🚫 not valid
  @reset: true  // 🚫 not valid
}

Of course you could quote any of the above invalid ones, but it takes something away from the fluidity and legibility when you have to do that.

@justin-schroeder justin-schroeder added 💡 needs ideas Open to new suggestions and ideas 🪄 enhancement New feature or request and removed 🐛 bug Verified bug by team labels Feb 14, 2022
@justin-schroeder
Copy link
Member

Wondering if it might be best to just call $reset a reserved word.

@andrew-boyd
Copy link
Member Author

Reserving $reset feels reasonable in light of the above. I’m in favor.

@luan-nk-nguyen
Copy link
Contributor

As long as it's consistent between the Vue component and the schema, I think this will be ok. $reset seems like it could be a relatively common collision within schema — I could see people using it for functions or other things — so we might consider ways to make it clear that it is a reserved word.

@justin-schroeder justin-schroeder added this to the Beta 5 milestone Feb 28, 2022
@justin-schroeder justin-schroeder added the 🔵 priority-3 3. Lower priority issue label Feb 28, 2022
@justin-schroeder
Copy link
Member

Ok, this is implemented for all -class and fooClass attributes. Caveat, it does not work for the classes prop which will require the prefix: __raw__classes

@justin-schroeder justin-schroeder added the 🚀 release-ready Feature or fix is complete and on an upcoming release branch label Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪄 enhancement New feature or request 💡 needs ideas Open to new suggestions and ideas 🔵 priority-3 3. Lower priority issue 🚀 release-ready Feature or fix is complete and on an upcoming release branch
Projects
None yet
Development

No branches or pull requests

3 participants