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

🐛 Masks model updates #4175

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

ekwoka
Copy link
Contributor

@ekwoka ekwoka commented Apr 26, 2024

Solves #3691

Problem

When a value modeled onto an input with x-mask is updated on the data side (not via the masked input itself), the mask is not processing and masking the value. Most commonly this will be common with $money where the programmatically updated value is likely to just be a raw number or number string.

Demo

Solution

This checks for and wraps the x-model forced update method to retrigger the masking on the value and push it back into the model.

Not exactly pretty. I can do a more significant refactoring of the directive to make this cleaner, but this basically shows what steps aren't being handled.

Tests

This includes a bespoke test, as well as passes a previous disabled test (which is now enabled) that seemed to run into this issue in a flaky manner

@calebporzio
Copy link
Collaborator

Hey @ekwoka - can you provide a clearer explanation of what this PR is doing? Thanks. (also, clever approach: decorating the forceModelUpdate method)

@ekwoka
Copy link
Contributor Author

ekwoka commented Apr 29, 2024

You got it! 👍

@Tim-Wils
Copy link

Tim-Wils commented May 2, 2024

@ekwoka Hey Eric, does this fix also fixes the bug with the native user-valid/user-invalid ? As per this discussion: #4046

@ekwoka
Copy link
Contributor Author

ekwoka commented May 2, 2024

It shouldn't impact that at all.

@Tim-Wils
Copy link

Tim-Wils commented May 2, 2024

Alright, that's a still a problem then. Thanks for this PR though!

@godismyjudge95
Copy link

Ran into this issue just now. Going to have to figure out an alternate solution until this PR is merged as I need the mask to apply both with manual and programmatic updates.

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.

None yet

4 participants