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

[Form] Add tel and color types #22679

Closed
wants to merge 11 commits into
base: 3.4
from

Conversation

@apetitpa
Contributor

apetitpa commented May 9, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #22596
License MIT
Doc PR not provided yet
Show outdated Hide outdated CONTRIBUTORS.md Outdated
use Symfony\Component\Form\AbstractType;
class TelType extends AbstractType

This comment has been minimized.

@ro0NL

ro0NL May 9, 2017

Contributor

i definitely prefer TelephoneType here 🤔

@ro0NL

ro0NL May 9, 2017

Contributor

i definitely prefer TelephoneType here 🤔

This comment has been minimized.

@apetitpa

apetitpa May 9, 2017

Contributor

I was quite unsure about what name to use, so I chose the one that matches the "tel" HTML5 input name but I have to admit it is less clear

@apetitpa

apetitpa May 9, 2017

Contributor

I was quite unsure about what name to use, so I chose the one that matches the "tel" HTML5 input name but I have to admit it is less clear

This comment has been minimized.

@ro0NL

ro0NL May 9, 2017

Contributor

i think fully qualified is preferred :)

@ro0NL

ro0NL May 9, 2017

Contributor

i think fully qualified is preferred :)

This comment has been minimized.

@apetitpa

apetitpa May 9, 2017

Contributor

I'll rename the class then thank you !

@apetitpa

apetitpa May 9, 2017

Contributor

I'll rename the class then thank you !

This comment has been minimized.

@ro0NL

ro0NL May 9, 2017

Contributor
@ro0NL

ro0NL May 9, 2017

Contributor

This comment has been minimized.

@apetitpa

apetitpa May 16, 2017

Contributor

Still no consensus about how the class should be named ? @fabpot @nicolas-grekas @xabbuh

@apetitpa

apetitpa May 16, 2017

Contributor

Still no consensus about how the class should be named ? @fabpot @nicolas-grekas @xabbuh

This comment has been minimized.

@apetitpa

apetitpa Sep 21, 2017

Contributor

Hello, I'd like to finish this PR, how should I name the class ? TelType, TelephoneType, PhoneType etc ? @symfony/deciders @fabpot

@apetitpa

apetitpa Sep 21, 2017

Contributor

Hello, I'd like to finish this PR, how should I name the class ? TelType, TelephoneType, PhoneType etc ? @symfony/deciders @fabpot

This comment has been minimized.

@ro0NL

ro0NL Sep 21, 2017

Contributor

either way no blocker for me :) i understand we just want type="tel" one way or another 👍 (my reasoning still stands though(.

@ro0NL

ro0NL Sep 21, 2017

Contributor

either way no blocker for me :) i understand we just want type="tel" one way or another 👍 (my reasoning still stands though(.

This comment has been minimized.

@jvasseur

jvasseur Sep 21, 2017

Contributor

I think we should go with types that match the html types because that's we we tend to do instead of trying to name them based of the semantic of the field (CheckboxType, TextareaType, RangeType are good examples of this).

@jvasseur

jvasseur Sep 21, 2017

Contributor

I think we should go with types that match the html types because that's we we tend to do instead of trying to name them based of the semantic of the field (CheckboxType, TextareaType, RangeType are good examples of this).

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 2, 2017

Member

@ro0NL I think your reasoning was wrong here when stating this (about "tel"):

we should not name our class after an implementation detail

HTML5 is exactly the opposite to an implementation detail: it's a public interface, a standard, the tip of the iceberg. So we should definitely not seek to be more clever than the ppl who wrote it (we're not ;) ).

@nicolas-grekas

nicolas-grekas Oct 2, 2017

Member

@ro0NL I think your reasoning was wrong here when stating this (about "tel"):

we should not name our class after an implementation detail

HTML5 is exactly the opposite to an implementation detail: it's a public interface, a standard, the tip of the iceberg. So we should definitely not seek to be more clever than the ppl who wrote it (we're not ;) ).

@ro0NL

ro0NL approved these changes May 9, 2017

@issei-m

This comment has been minimized.

Show comment
Hide comment
@issei-m

issei-m May 9, 2017

Contributor

Looks good😀
btw would "TelephoneType" be better to be named "TelType" in accordance with type-name of html5 tag? (I'm not familiar with English, so not sure the abbr like "tel" is common used though.)

Contributor

issei-m commented May 9, 2017

Looks good😀
btw would "TelephoneType" be better to be named "TelType" in accordance with type-name of html5 tag? (I'm not familiar with English, so not sure the abbr like "tel" is common used though.)

@issei-m

This comment has been minimized.

Show comment
Hide comment
@issei-m

issei-m May 9, 2017

Contributor

would "TelephoneType" be better to be named "TelType" in accordance with type-name of html5 tag?

Oops, I've just missed the review-comment by @ro0NL. Never mind this comment.

Contributor

issei-m commented May 9, 2017

would "TelephoneType" be better to be named "TelType" in accordance with type-name of html5 tag?

Oops, I've just missed the review-comment by @ro0NL. Never mind this comment.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 9, 2017

Contributor

my reasoning; tel === telephone, so TelephoneType is explicit/verbose. In my domain i'd also call this field $user->telephone, not $user->tel.

Contributor

ro0NL commented May 9, 2017

my reasoning; tel === telephone, so TelephoneType is explicit/verbose. In my domain i'd also call this field $user->telephone, not $user->tel.

@lyrixx lyrixx changed the title from [Form] Add tel and color types to [Form] Add telephone and color types May 11, 2017

@nicolas-grekas nicolas-grekas changed the base branch from master to 3.4 May 23, 2017

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 21, 2017

Member

Needs rebase to fix the conflict. (Let's keep TelephoneType btw, in case there is still debate)

Member

ogizanagi commented Sep 21, 2017

Needs rebase to fix the conflict. (Let's keep TelephoneType btw, in case there is still debate)

@fabpot

fabpot approved these changes Oct 1, 2017

@apetitpa

This comment has been minimized.

Show comment
Hide comment
@apetitpa

apetitpa Oct 1, 2017

Contributor

@ogizanagi @fabpot rebased with 3.4 (sorry for the delay)

Contributor

apetitpa commented Oct 1, 2017

@ogizanagi @fabpot rebased with 3.4 (sorry for the delay)

@fabpot

fabpot approved these changes Oct 1, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 1, 2017

Member

@apetitpa Can you have a look at the tests, they seem to be broken by your changes.

Member

fabpot commented Oct 1, 2017

@apetitpa Can you have a look at the tests, they seem to be broken by your changes.

apetitpa added some commits May 9, 2017

apetitpa added some commits May 9, 2017

@apetitpa

This comment has been minimized.

Show comment
Hide comment
@apetitpa

apetitpa Oct 2, 2017

Contributor

@fabpot It's done, it's green again. Travis 7.1 failing test will be fixed when merging.

Contributor

apetitpa commented Oct 2, 2017

@fabpot It's done, it's green again. Travis 7.1 failing test will be fixed when merging.

@apetitpa

This comment has been minimized.

Show comment
Hide comment
@apetitpa

apetitpa Oct 2, 2017

Contributor

@nicolas-grekas It's done but I still have one concern regarding the name of the TelephoneType class. Should we keep this name or use the original suggested name which was TelType as the HTML input type

Contributor

apetitpa commented Oct 2, 2017

@nicolas-grekas It's done but I still have one concern regarding the name of the TelephoneType class. Should we keep this name or use the original suggested name which was TelType as the HTML input type

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 2, 2017

Member

If HTML5 is fine with "tel", so am I.

Member

nicolas-grekas commented Oct 2, 2017

If HTML5 is fine with "tel", so am I.

@apetitpa apetitpa changed the title from [Form] Add telephone and color types to [Form] Add tel and color types Oct 2, 2017

@apetitpa

This comment has been minimized.

Show comment
Hide comment
@apetitpa

apetitpa Oct 2, 2017

Contributor

@nicolas-grekas Done, reverted to TelType as agreed

Contributor

apetitpa commented Oct 2, 2017

@nicolas-grekas Done, reverted to TelType as agreed

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas
Member

nicolas-grekas commented Oct 2, 2017

Thank you @apetitpa.

@apetitpa apetitpa deleted the apetitpa:add-form-types branch Oct 2, 2017

This was referenced Oct 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment