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] Changed UrlType input type to text when default_protocol is not null #29691

Closed
wants to merge 5 commits into
base: 3.4
from

Conversation

Projects
None yet
7 participants
@MatTheCat
Copy link
Contributor

MatTheCat commented Dec 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
@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Dec 28, 2018

@MatTheCat it looks like some tests need to be fixed

Status: Needs work

@xabbuh xabbuh added this to the 3.4 milestone Dec 28, 2018

@MatTheCat

This comment has been minimized.

Copy link
Contributor

MatTheCat commented Dec 28, 2018

@xabbuh I don't understand what's going on. I see tests failing on Symfony\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.

@MatTheCat MatTheCat force-pushed the MatTheCat:ticket_29690 branch from 666b83e to ea5ad33 Jan 3, 2019

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Jan 3, 2019

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

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

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Jan 3, 2019

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

@MatTheCat

This comment has been minimized.

Copy link
Contributor

MatTheCat commented Jan 3, 2019

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

@MatTheCat MatTheCat force-pushed the MatTheCat:ticket_29690 branch 2 times, most recently from b69135a to 0e5ae94 Jan 3, 2019

@MatTheCat

This comment has been minimized.

Copy link
Contributor

MatTheCat commented Jan 7, 2019

@xabbuh what can I do to get this merged?

@xabbuh

xabbuh approved these changes Jan 7, 2019

@xabbuh xabbuh force-pushed the MatTheCat:ticket_29690 branch from 0e5ae94 to d907049 Jan 7, 2019

@xabbuh xabbuh force-pushed the MatTheCat:ticket_29690 branch from dcb6db0 to d171448 Jan 7, 2019

@MatTheCat MatTheCat force-pushed the MatTheCat:ticket_29690 branch from 3917e1b to 1841b5d Jan 8, 2019

@MatTheCat

This comment has been minimized.

Copy link
Contributor

MatTheCat commented Jan 8, 2019

@stof is it good now?

@ro0NL

ro0NL approved these changes Jan 8, 2019

@MatTheCat MatTheCat force-pushed the MatTheCat:ticket_29690 branch from 3a9b0d8 to b9893e4 Jan 10, 2019

@MatTheCat MatTheCat force-pushed the MatTheCat:ticket_29690 branch from b9893e4 to acd3c4c Jan 14, 2019

@MatTheCat

This comment has been minimized.

Copy link
Contributor

MatTheCat commented Jan 14, 2019

@stof ?

@MatTheCat MatTheCat force-pushed the MatTheCat:ticket_29690 branch from acd3c4c to 2f05174 Jan 16, 2019

@MatTheCat MatTheCat force-pushed the MatTheCat:ticket_29690 branch from 2f05174 to 3411766 Jan 16, 2019

@MatTheCat

This comment has been minimized.

Copy link
Contributor

MatTheCat commented Jan 17, 2019

@fabpot could this PR be merged please?

@MatTheCat MatTheCat closed this Jan 18, 2019

@mablae

This comment has been minimized.

Copy link
Contributor

mablae commented Jan 18, 2019

@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!

@MatTheCat

This comment has been minimized.

Copy link
Contributor

MatTheCat commented Jan 22, 2019

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

@mablae

This comment has been minimized.

Copy link
Contributor

mablae commented Jan 22, 2019

You're wrong. You already got positive feedback.

@MatTheCat

This comment has been minimized.

Copy link
Contributor

MatTheCat commented Jan 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?

@mablae

This comment has been minimized.

Copy link
Contributor

mablae commented Jan 22, 2019

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