-
Notifications
You must be signed in to change notification settings - Fork 2
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
Input Masking (Meta #69) #76
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcwieland95 I only have very minimal information regarding the strategy with how we set the locale here. I can't really judge the approach and added my questions for it. I'm going to approve this anyway, even though I think we are choosing the wrong approach here.
Furthermore, commit 8a1eab4 should be squashed in the later one. I don't see a reason to have it in the history, then immediately reverse/change it in the next commit.
scale: attr.scale is defined ? attr.scale : null, | ||
radix: attr.radix is defined ? attr.radix : null, | ||
thousandsSeparator: attr.thousandsSeparator is defined ? attr.thousandsSeparator : null, | ||
normalizeZeros: attr.normalizeZeros is defined ? attr.normalizeZeros : null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easier to read syntax with default filter:
scale: attr.scale|default(null),
radix: attr.radix|default(null),
thousandsSeparator: attr.thousandsSeparator|default(null),
normalizeZeros: attr.normalizeZeros|default(null)
mask: attr.mask|default('Number'), | ||
scale: attr.scale|default(2), | ||
scale: attr.radix|default('.'), | ||
thousandsSeparator: attr.thousandsSeparator|default('\''), | ||
normalizeZeros: attr.normalizeZeros|default(false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarifying question: Why do we need to configure these manually for the field instead of being controlled by the locale that is set for the HTML document? There is a whole browser API for locales including measurements, thousand separators, that is controlled by what locale/lang is set for the HTML document, and what locale the user uses in their browser. Seems counterintuitive to not use the native possibilities of the browser for controlling these settings. Because this can be set once for the whole document via HTML lang
, and then all fields should automatically apply the set locale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@renestalder I didn't play around to see how correctly the browser settings are working. But the fact is that we can't force it. We define certain stylings in the Formatter and want them to align in the frontend as well.
In the issue there's a note on that:
Je nach Spracheinstellung vom Browser wird das Komma oder der Punkt gesetzt. Darüber haben wir aber keinen Einfluss. Entweder es funktioniert einfach für den Benutzer mit den korrekten Einstellungen oder man muss es manuell anpassen. In unserem Fall haben wir aber genaue Einstellungen im Form Builder, die wir forcieren wollen. Somit müssen wir es mit JavaScript überschreiben.
https://dev.whatwedo.ch/araise/araise-meta/-/issues/69#note_382677
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the fact is that we can't force it
And we shouldn't!
User should see formatting based on their browser locale, not based on the application. Underneath the fields anyway send the value always in the same format. What the UI shows to the user is then converted by the browser based on their settings. That's how it should work. I don't understand the reasoning behind the enforcing, and the ergonomics of the developers needing to set this explicitly on individual components.
Using the locale sensitive features of the browser not only enables the correct time and date formatting for the locale the user prefers, it also enables other language features to correctly apply, like quotations marks.
It's really difficult to tell if this solution here is not a foot gun. I feel like the ticket missing the opportunity to really research the problem mentioned and whether this is really an application issue or the expected behaviour. I don't see a single definition in the discussion of what languages and locales the people reporting this are using.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@renestalder We have to sync the frontend with the backend. Having two different types of formatting is confusing for the user. Here we're talking about the "edit" view (input field) and the "view" view (backend formatted output). I don't think there we can just set languages and it gives us the correct formatting. At least Symfony isn't configuring it in that way. Things have to be set as we do now with the input masking.
This Stimulus controller can do all kind of things. Like forcing a special input style (phone numbers or AHV numbers). So the use case is very open. With the mentioned issue in mind we also enforce that on the money field type. This can be removed if we think it doesn't make sense and doesn't support a wider use-case. But we have to discuss this between front- and backend. We can add this to the list to be discussed.
ddd0938
to
5051a5e
Compare
No description provided.