NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Form] Changed UrlType input type to text when default_protocol is not null#29691

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

Jump to bottom

Closed

MatTheCat wants to merge5 commits intosymfony:3.4fromMatTheCat:ticket_29690

Closed

[Form] Changed UrlType input type to text when default_protocol is not null#29691

MatTheCat wants to merge5 commits intosymfony:3.4fromMatTheCat:ticket_29690

Conversation

Copy link

Contributor

MatTheCat commentedDec 26, 2018

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#29690
License	MIT

carsonbot added Status: Needs Review Form Bug labels

Dec 26, 2018

Copy link

Member

xabbuh commentedDec 28, 2018

@MatTheCat it looks like some tests need to be fixed

Status: Needs work

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Dec 28, 2018

xabbuh added this to the3.4 milestone

Dec 28, 2018

Copy link

ContributorAuthor

MatTheCat commentedDec 28, 2018•
edited
Loading

@xabbuh I don't understand what's going on. I see tests failing onSymfony\Bridge\Twig\Tests\Extension\FormExtensionBootstrap3HorizontalLayoutTest::testUrl but this test doesn't exist anymore in this PR. Why do I see it?

Tests pass on my local branch.

Copy link

Contributor

ro0NL commentedJan 3, 2019

we might gain some extra value to useinputmode=url when usingtype=text. AFAIK this will force a specific keyboard on mobile (e.g. URL input).

https://caniuse.com/#feat=input-inputmode

Copy link

Contributor

ro0NL commentedJan 3, 2019

tested on android with built-in samsung keyboard, and works as expected 👍

Copy link

ContributorAuthor

MatTheCat commentedJan 3, 2019

Don't know if failing tests prevent to merge this?

Copy link

ContributorAuthor

MatTheCat commentedJan 7, 2019

@xabbuh what can I do to get this merged?

xabbuh approved these changes

Jan 7, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Jan 7, 2019

nicolas-grekas approved these changes

Jan 7, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels

Jan 7, 2019

stof requested changes

Jan 7, 2019

View reviewed changes

src/Symfony/Bridge/Twig/Tests/Extension/FormExtensionTableLayoutTest.php OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

MatTheCat commentedJan 8, 2019

@stof is it good now?

ro0NL approved these changes

Jan 8, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Jan 8, 2019

Copy link

ContributorAuthor

MatTheCat commentedJan 14, 2019

@stof ?

Mathieu Lechatand others added5 commits

January 16, 2019 16:21

[Form] Changed UrlType input type to text when default_protocol is no…

161d575

…t null

fix lowest version constraints

1007fc2

Add inputmode attribute

09675b6

Test inputmode too

e23845a

Update CS

3411766

Copy link

ContributorAuthor

MatTheCat commentedJan 17, 2019

@fabpot could this PR be merged please?

MatTheCat closed this

Jan 18, 2019

Copy link

Contributor

mablae commentedJan 18, 2019•
edited
Loading

@MatTheCat If you need to use your branch NOW, you could alias it via composer.json.
Here is shown how it is done:https://lornajane.net/posts/2014/use-a-github-branch-as-a-composer-dependency

Pushing for merge and mentioning fabpot seldom results in a faster merge. If the feature is good it will be merged. Please be a little more patient after putting lot of work into your PR.

And thank you for the work!

xabbuh mentioned this pull request

Jan 18, 2019

[Form] Changed UrlType input type to text when default_protocol is not null#29926

Merged

MatTheCat deleted the ticket_29690 branch

January 18, 2019 11:53

Copy link

ContributorAuthor

MatTheCat commentedJan 22, 2019

I conclude the feature is not good so there's no point for me to continue rebasing my branch.

Copy link

Contributor

mablae commentedJan 22, 2019

You're wrong. You already got positive feedback.

Copy link

ContributorAuthor

MatTheCat commentedJan 22, 2019

Dude don't provoke me on my personal email. You're point is a PR is merged if it's good right? This one didn't.

Can we stop the conversation here?

Copy link

Contributor

mablae commentedJan 22, 2019 via email

good luck with that attitude.It is too sad, since your feature was pretty good. Your PR was alsoreferred 4 days ago,so why are you on such a hurry?I am glad to help you setup your composer.json to use your branch NOW asmentioned.Best regardsMalteAm Di., 22. Jan. 2019 um 13:19 Uhr schrieb Mathieu <notifications@github.com

…

: Dude don't provoke me on my personal email. You're point is a PR is merged if it's good right? This one didn't. Can we stop the conversation here? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#29691 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXw8O217I8ZpvkYpHQ_JklTLbDnbw8Vks5vFwHfgaJpZM4Zh5Pn> .

nicolas-grekas added a commit that referenced this pull request

Jan 25, 2019

bug#29926[Form] Changed UrlType input type to text when default_pro…

e3b010f

…tocol is not null (MatTheCat)This PR was merged into the 3.4 branch.Discussion----------[Form] Changed UrlType input type to text when default_protocol is not null| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#29690| License       | MIT| Doc PR        |replaces#29691Commits-------2791edf [Form] Changed UrlType input type to text when default_protocol is not null