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

Price field #1

Merged
merged 11 commits into from
Dec 8, 2023
Merged

Price field #1

merged 11 commits into from
Dec 8, 2023

Conversation

michaelberkhahn
Copy link
Collaborator

Test in showcase

Copy link
Collaborator

@user-23 user-23 left a comment

Choose a reason for hiding this comment

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

Ob hier das Rad neu erfunden wird?
Ziemlich viel Currency/Number-Formatierungs-Logik.
Sollte das nicht die react-intl lib übernehmen?

  • Tests für die format/sanitize Methoden wären gut
  • Changelog fehlt im PR

@michaelberkhahn
Copy link
Collaborator Author

Ob hier das Rad neu erfunden wird?
Nein, absolut nicht. Weder MUI hat ein PriceField noch sind andere existierende Bibliotheken in der Lage ein fehlerfreies PriceField, welches unsere Anforderungen erfüllt, anzubieten. Ich habe mehrere getestet.

Sollte das nicht die react-intl Lib übernehmen?
Leider nein. Das war auch mein erster Ansatz aber dann muss das PriceField dynamisch eine eigene Intl-Instanz erzeugen, was wir als "zu heftig" empfunden haben, da wir ja eigentlich nur Formatierungen davon benötigen, die uns auch 1 zu 1 die das native Intl des Browsers zur Verfügung stellt (toLocaleString nutzt genau das)

  • wir geben ja Props rein, die sich vom aktuellen Intl-Wrapper unterscheiden können
  • ich will also in einer App sowohl das DE aber auch das UK Währungsformat haben
  • der aktuelle Intl-Wrapper um die App drum herum lässt aber nach der Initialisierung keine Änderung des locale mehr zu
  • des Weiteren bietet react-intl nur Formatierungsmethoden an aber die neue Komponente basiert zu 70% auf Validierung ... Formatierung ist nur ein kleiner Teil davon
  • und react-intl basiert letzendlich auch nur auf dem nativen Intl des Browsers, welches wir in der Komponente direkt ansprechen (aber die Flexibilität des Locale-Wechsels haben)

Changelog fehlt im PR => guter punkt erweitern wir

Tests für die format/sanitize Methoden wären gut => finde ich prinzipiell auch gut aber habe ich noch nie für den core gemacht... gibt es da eigentlich schon irgendeinen test? kannst uns da ja auch gerne untersützen ;)

@theRealPengBang theRealPengBang merged commit 5d8bd10 into master Dec 8, 2023
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.

3 participants