Skip to content

[Form] Added "html5" option to both MoneyType and PercentType #35956

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 24, 2020
Merged

[Form] Added "html5" option to both MoneyType and PercentType #35956

merged 1 commit into from
Sep 24, 2020

Conversation

romaricdrigon
Copy link
Contributor

@romaricdrigon romaricdrigon commented Mar 4, 2020 •
edited
Loading

Q A
Branch? master for features
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR TBD

Hello,

In the same way that NumberType offers a html5 option to render a number input instead of a text input, this PR adds the same option to both MoneyType and PercentType. Number inputs offer a better UI, especially on mobile (specific keyboard), and they accept extra HTML attributes, such as max=100, which are quite useful.

The challenge is that number inputs need a "raw" value, nor formatted nor localized.
Format is described here https://www.w3.org/TR/html51/sec-forms.html#date-time-and-number-formats (non-normative, but tested on a few browsers). It matches number formatting for en locale (which is great, since it is the one provided by Symfony Intl polyflil), without grouping.

Implementation was done in a manner similar to NumberType for MoneyType.
PercentType required to modify PercentToLocalizedStringTransformer too.

As a bonus, PercentType had no tests at all, I added a few.

I wanted to get some feedback on the idea first, remaining steps:

  • update CHANGELOG.md
  • update the doc

@romaricdrigon romaricdrigon requested a review from xabbuh as a code owner March 4, 2020 10:51
@romaricdrigon romaricdrigon force-pushed the feat-money-percent-html5 branch from e337ce9 to 3c686f2 Compare March 4, 2020 10:53
@nicolas-grekas nicolas-grekas added this to the next milestone Mar 4, 2020
@nicolas-grekas
Copy link
Member

We need to be careful here, type=number is increasingly seen as providing bad UX.
E.g. https://bradfrost.com/blog/post/you-probably-dont-need-input-typenumber/

@romaricdrigon
Copy link
Contributor Author

Thats' a good point, and actually it is specified in the norm that they should not be used for non-sequential inputs (ie., bank account numbers...). Maybe it would be worth a warning in NumberType documentation?

Fortunately here with monetary amounts & percentages we should be safe, there are legit use cases.

@nicolas-grekas
Copy link
Member

(rebase needed)

@xabbuh xabbuh modified the milestones: next, 5.1 May 5, 2020
@romaricdrigon romaricdrigon force-pushed the feat-money-percent-html5 branch 3 times, most recently from e30e36e to 8efe194 Compare May 6, 2020 16:11
@romaricdrigon
Copy link
Contributor Author

Thanks you for the heads up, I rebased code & updated CHANGELOG.md
I will work on a PR for documentation tomorrow.

@romaricdrigon romaricdrigon force-pushed the feat-money-percent-html5 branch 7 times, most recently from a97942d to e079a9a Compare May 6, 2020 22:18
@romaricdrigon
Copy link
Contributor Author

Fabbot and Travis are green, AppVeyor failed because of a quota. If someone can please restart it, it should pass.

xabbuh
xabbuh reviewed May 8, 2020
View reviewed changes
...ymfony/Component/Form/Extension/Core/DataTransformer/PercentToLocalizedStringTransformer.php
*
* @throws UnexpectedTypeException if the given value of type is unknown
*/
public function __construct(int $scale = null, string $type = null, ?int $roundingMode = null)
public function __construct(int $scale = null, string $type = null, ?int $roundingMode = null, bool $html5Format = false)
Copy link
Member

Choose a reason for hiding this comment

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

I think I would rather allow passing a $locale and $grouping argument like in the NumberToLocalizedStringTransformer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have missed feelings about that: HTML5 format happens to be en locale without grouping, but it's accidental. I feel like it is correct to expose clearly that we want HTML5-compatible format in the normalizer interface.

@romaricdrigon romaricdrigon force-pushed the feat-money-percent-html5 branch from e079a9a to 9e4ea16 Compare May 26, 2020 08:38
@fabpot
Copy link
Member

fabpot commented Sep 1, 2020

What's the status of this PR?

@romaricdrigon
Copy link
Contributor Author

For me, the code part was done. I was waiting for some "formal" approval before doing the doc update.
Everything can be over pretty quickly, I think that addition is still relevant

@nicolas-grekas nicolas-grekas force-pushed the feat-money-percent-html5 branch from 9e4ea16 to 935dfdb Compare September 24, 2020 13:37
@nicolas-grekas nicolas-grekas force-pushed the feat-money-percent-html5 branch from 935dfdb to f7312a4 Compare September 24, 2020 13:49
nicolas-grekas
nicolas-grekas approved these changes Sep 24, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Works for me (I rebased + made some minor tweaks)

@fabpot
Copy link
Member

fabpot commented Sep 24, 2020

Thank you @romaricdrigon.

@fabpot fabpot mentioned this pull request Sep 24, 2020
Closed
@fabpot fabpot merged commit 6e9949b into symfony:master Sep 24, 2020
@romaricdrigon
Copy link
Contributor Author

Works for me (I rebased + made some minor tweaks)

Thanks you very much @nicolas-grekas for finishing the PR!
I got supra busy early September and less active, but I'm happy to see this ready for 5.2. I will work on the documentation PR.

@romaricdrigon romaricdrigon deleted the feat-money-percent-html5 branch September 28, 2020 15:21
@romaricdrigon romaricdrigon mentioned this pull request Sep 28, 2020
Merged
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Oct 2, 2020
@javiereguiluz
minor #14295 [Form] Added documentation for "html5" option of MoneyTy…
b748085 
…pe and PercentType (romaricdrigon)

This PR was merged into the master branch.

Discussion
----------

[Form] Added documentation for "html5" option of MoneyType and PercentType

Hello,

Following symfony/symfony/pull/35956, which was merged for 5.2, this PR adds the corresponding documentation.

Commits
-------

d8fc70d [Form] Added documentation for "html5" option of MoneyType and PercentType
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabbuh xabbuh xabbuh left review comments

@javiereguiluz javiereguiluz javiereguiluz approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Feature Form Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
6 participants
@romaricdrigon @nicolas-grekas @fabpot @javiereguiluz @xabbuh @carsonbot