8000 [Form] Add the html5 option to ColorType to validate the input by fancyweb · Pull Request #36302 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Add the html5 option to ColorType to validate the input#36302

Merged
fabpot merged 1 commit intosymfony:masterfrom
fancyweb:form-color-constraint
May 5, 2020
Merged

[Form] Add the html5 option to ColorType to validate the input#36302
fabpot merged 1 commit intosymfony:masterfrom
fancyweb:form-color-constraint

Conversation

@fancyweb
Copy link
Contributor
@fancyweb fancyweb commented Apr 1, 2020
Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets no
License MIT
Doc PR TODO

Continuation of #35626.

I'm resubmitting the initial implementation, this time in the Form component.

This Color constraint is dedicated to the HTML5 input type="color".

@fancyweb fancyweb requested a review from xabbuh as a code owner April 1, 2020 15:28
@fancyweb fancyweb mentioned this pull request Apr 1, 2020
Closed
HeahDude
HeahDude reviewed Apr 1, 2020
View reviewed changes
Copy link
Contributor
@HeahDude HeahDude left a comment

Choose a reason for hiding this comment

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

What about a strict_html5 option in a ColorValidatorTypeExtension for the ColorType that adds it with a normalizer automatically?

@stof
Copy link
Member
stof commented Apr 1, 2020

Constraints should not be implemented in the Form component IMO.

@HeahDude
Copy link
Contributor
HeahDude commented Apr 1, 2020

So a solution could be to use the normalizer to add a Regex constraint when the option is true.

@HeahDude
Copy link
Contributor
HeahDude commented Apr 1, 2020

So a solution could be to use the normalizer to add a Regex constraint when the option is true.

But then we should not require the Validator component for such option, add do it in the ColorType directly using a preg_match in a listener instead.
8000

@xabbuh
Copy link
Member
xabbuh commented Apr 2, 2020

I agree, this could simply be part of the ColorType. For consistency with other form types I suggest to add an html5 option which renders an HTML5 input="type" widget and performs these validation rules.

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 3, 2020
@fancyweb fancyweb force-pushed the form-color-constraint branch 2 times, most recently from 31b82a2 to 0f352fb Compare April 8, 2020 10:20
@fancyweb fancyweb changed the title [Form] Add the color constraint [Form] Add the html5 option to ColorType to validate the input Apr 8, 2020
@fancyweb
Copy link
Contributor Author
fancyweb commented Apr 8, 2020 •
edited
Loading

I took your ideas into consideration. That's kind of out of my initial use case but it's still going to be useful.

which renders an HTML5 input="type" widget

AFAIK, that's already the case.

@fancyweb
Copy link
Contributor Author
fancyweb commented Apr 8, 2020

do it in the ColorType directly using a preg_match in a listener instead.

Should we however add a RegExp constraint (and skip the listener) if the Validator component is there?

xabbuh
xabbuh reviewed Apr 8, 2020
View reviewed changes
@fancyweb fancyweb force-pushed the form-color-constraint branch from 0f352fb to caead2d Compare April 8, 2020 12:21
HeahDude
HeahDude suggested changes Apr 8, 2020
View reviewed changes
@fancyweb fancyweb force-pushed the form-color-constraint branch 2 times, most recently from 889c936 to 56fd5d5 Compare April 14, 2020 12:11
xabbuh
xabbuh reviewed Apr 16, 2020
View reviewed changes
@fancyweb fancyweb force-pushed the form-color-constraint branch from 56fd5d5 to 454b6ff Compare April 16, 2020 15:25
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot
Copy link
Member
fabpot commented May 5, 2020

Thank you @fancyweb.

@fabpot fabpot mentioned this pull request May 5, 2020
Closed
@fabpot fabpot merged commit 341ea45 into symfony:master May 5, 2020
@fancyweb fancyweb deleted the form-color-constraint branch May 5, 2020 06:52
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
@mhujer mhujer mentioned this pull request May 28, 2020
Merged
@YaFou YaFou mentioned this pull request Jun 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

@xabbuh xabbuh xabbuh approved these changes

+1 more reviewer

@HeahDude HeahDude HeahDude requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature Form Status: Reviewed

Projects

None yet

Milestone

5.1

Development

Successfully merging this pull request may close these issues.

7 participants

@fancyweb @stof @HeahDude @xabbuh @fabpot @nicolas-grekas @carsonbot
0