Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Validator] AddedCssColor constraint#40168

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:5.4fromwelcoMattic:hexadecimal-constraint
Oct 6, 2021

Conversation

@welcoMattic
Copy link
Member

@welcoMatticwelcoMattic commentedFeb 12, 2021•
edited
Loading

QA
Branch?5.4
Bug fix?no
New feature?yes
Deprecations?no
Tickets
LicenseMIT
Doc PRsymfony/symfony-docs#14965

This PR introduces a newCssColor constraint. It comes with 3 validation modes:

  • Long, which allows all hexadecimal representation of a color, or 9 (#EEEEEEFF) characters
  • Short, which only allows hexadecimal colors on 4 (#EEE), 5 (#FFF00) characters
  • Named colors, which matchesthe official list of named colors
  • HTML5, which allows hexadecimal colors on 7 (#EEEEEE) characters as well as the HTML5 input type color

I know that such a color validation already exists in Symfony (in theColorType class), but it's hardcoded in the FormType, and not usable as an assert Annotation. We could decide to remove this hardcoded validation in favor of the new addedCssColor constraint. Let me know, if yes, I will make the change.

sstok, norkunas, and RSalo reacted with thumbs up emojisstok reacted with eyes emoji
@norkunas
Copy link
Contributor

Why only hex? Maybe Color constraint with hex, rgb(a),hsl(a) support would be better :)

@welcoMattic
Copy link
MemberAuthor

@norkunas Yes, I thought about it. But I was not sure if we have to support hex,rgb(a),hsl(a) in the same Constraint. I think that we should make separate Constraints to avoid to guess the format. WDYT?

@norkunas
Copy link
Contributor

Also good idea :)

OskarStark reacted with thumbs up emoji

@javiereguiluz
Copy link
Member

javiereguiluz commentedFeb 12, 2021•
edited
Loading

Actually, CSS 4 defines four kinds of hexadecimal colors (https://www.w3.org/TR/css-color-4/#typedef-hex-color):

  • 6-digit:#00ff00
  • 8-digit:#0000ffcc
  • 3-digit:#0f0
  • 4-digit:#0f0c
OskarStark and chapterjason reacted with thumbs up emojisstok reacted with eyes emoji

@xabbuhxabbuh added this to the5.x milestoneFeb 12, 2021
@fancyweb
Copy link
Contributor

Interesting previous discussions on#35626

@OskarStark
Copy link
Contributor

I would prefer HexColor instead of HexaColor

@javiereguiluz
Copy link
Member

Here are some comments in case we want to move this forward:

  • I'd renameHexaColor toCssColor, because this constraint is only about CSS-valid colors
  • CssColor would also match Symfony's primary target: the web (and we use CSS colors even when outside the web; e.g.Symfony's Console true colors are defined with some of the CSS-valid formats)
  • Even if it's calledCssColor we don't need to support from the beginning all current (and future)ways of defining CSS colors. This is the same asCardScheme constraint: it only supported a few of the most popular cards at first and, in the following years, we added more cards. Even if it's calledCardScheme, it doesn't support all the existing card schemes yet ... but that's fine; it's still very useful and widely used (so, we can add aCssColor without supporting ALL color formats at first, specially the less used ones)
welcoMattic, apfelbox, and sstok reacted with thumbs up emoji

@fabpot
Copy link
Member

@welcoMattic How can we move forward?

@welcoMattic
Copy link
MemberAuthor

@fabpot I intend to apply Javier's comments, and make a first version of this constraint that we can improve later following user's needs.

OskarStark and sstok reacted with thumbs up emoji

@welcoMatticwelcoMatticforce-pushed thehexadecimal-constraint branch 3 times, most recently fromcf0bba7 tocead49aCompareJuly 4, 2021 17:08
@welcoMattic
Copy link
MemberAuthor

welcoMattic commentedJul 4, 2021•
edited
Loading

I addressed@javiereguiluz's comments about making a first usable version of CssColor Constraint, with common formats support, which are:

  • 6-digit:#00ff00
  • 8-digit:#0000ffcc
  • 3-digit:#0f0
  • 4-digit:#0f0c
  • basic named colors (black|red|green|yellow|blue|magenta|cyan|white) (if required for 1st version, we could copy pastethe official list of named colors)

Let me know if we are agree on this 1st version

@derrabusderrabus changed the title[Validator] Added HexaColor constraint[Validator] Added CssColor constraintJul 4, 2021
Copy link
Member

@javiereguiluzjaviereguiluz left a comment

Choose a reason for hiding this comment

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

Mathieu, you've done a fantastic job here. This constraint is much more complete than I imagined. It's great!

I left some minor comments ... and I also wonder ifmodes is the best word to describe the different types of CSS colors allowed by the constraint. I can't suggest a better alternative, butmodes doesn't feel 100% correct to me.

Thanks!

welcoMattic reacted with heart emoji
@welcoMattic
Copy link
MemberAuthor

welcoMattic commentedSep 29, 2021•
edited
Loading

@javiereguiluz thanks! About$modes, what do you think about renaming it to$formats?
I've addressed all other comments, thanks for the review

status: needs review

@welcoMatticwelcoMatticforce-pushed thehexadecimal-constraint branch 4 times, most recently from347b2a1 to781598aCompareSeptember 29, 2021 21:12
@welcoMatticwelcoMatticforce-pushed thehexadecimal-constraint branch 2 times, most recently froma741651 to23709e7CompareSeptember 29, 2021 21:19
@welcoMattic
Copy link
MemberAuthor

Fabbot is wrong about the italian typo correction, it'spossibile and notpossible.

derrabus reacted with thumbs up emoji

@welcoMattic
Copy link
MemberAuthor

@javiereguiluz@derrabus I don't know exactly when the feature freeze will happen, but I think this PR is ready to be merged in 5.4.

@derrabus
Copy link
Member

I agree, this PR is ready. But I need a second approval to merge a feature PR.

@fabpot
Copy link
Member

Thank you@welcoMattic.

welcoMattic reacted with thumbs up emojiderrabus reacted with hooray emoji

@fabpotfabpot merged commit428434c intosymfony:5.4Oct 6, 2021
OskarStark added a commit to symfony/symfony-docs that referenced this pull requestOct 8, 2021
This PR was merged into the 5.4 branch.Discussion----------[Validator] Added CssColor constraintThis PR introduces the new `CssColor` constraint, contributed insymfony/symfony#40168Commits-------fc1539c Added CssColor constraint
@welcoMatticwelcoMattic deleted the hexadecimal-constraint branchOctober 8, 2021 09:50
*/
publicfunction__construct($formats,string$message =null,array$groups =null,$payload =null,array$options =null)
{
$validationModesAsString =array_reduce(self::$validationModes,function ($carry,$value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, I'm reading this PR and wondering if it wouldn't be a good idea to type things as much as we can when doing a chance?

Since Symfony 6 has better typing support, wouldn't it be a good idea to require adding typing when submitting a PR ?

Thanks

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

It could be great, but in this case I made a little mistake, thisarray_reduce is overengineered, it has to be aimplode call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at it twice now, and indeed, it could be replace with an implode :)

welcoMattic reacted with thumbs up emoji
This was referencedNov 5, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz left review comments

@stofstofstof left review comments

@jderussejderussejderusse left review comments

@OskarStarkOskarStarkOskarStark left review comments

@fabpotfabpotfabpot approved these changes

@derrabusderrabusderrabus approved these changes

+4 more reviewers

@drupoldrupoldrupol left review comments

@localheinzlocalheinzlocalheinz left review comments

@norkunasnorkunasnorkunas left review comments

@YaFouYaFouYaFou left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

14 participants

@welcoMattic@norkunas@javiereguiluz@fancyweb@OskarStark@fabpot@derrabus@drupol@stof@jderusse@localheinz@YaFou@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp