Skip to content
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

Add a range field and support steps in text fields #521

Merged
merged 13 commits into from
Jan 8, 2020

Conversation

fritzmg
Copy link
Contributor

@fritzmg fritzmg commented Jun 16, 2019

This PR adds a range input field to the form generator.

It also allows the step value to be set for number input fields.

I still used the old method of re-using the minlength and maxlength fields - so it still needs to be adjusted when #437 gets implemented.

aschempp
aschempp previously approved these changes Jun 17, 2019
@Toflar Toflar changed the title add range field and support step in text field [WIP] Add range field and support step in text field Jun 17, 2019
@Toflar
Copy link
Member

Toflar commented Jun 17, 2019

I've added the [WIP] prefix so the PR does not get merged in it's current state as it depends on #437.

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 16, 2019

I have updated the PR using minval and maxval. Should I rebase this to 4.8?

@leofeyer leofeyer added this to the 4.9 milestone Jul 16, 2019
@leofeyer
Copy link
Member

No, Contao 4.8 is feature complete, so you can leave the branch as it is.

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 16, 2019

Oh that's too bad. I only made the changes just now, because the depending change for 4.8 was made just now: #437 (comment)

@fritzmg fritzmg changed the title [WIP] Add range field and support step in text field Add range field and support step in text field Jul 16, 2019
@leofeyer
Copy link
Member

@fritzmg This PR needs to be rebased onto the current master branch so we can review it. 🙈

@fritzmg
Copy link
Contributor Author

fritzmg commented Dec 24, 2019

Rebased and ready to review.

Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

One minor detail, LGTM otherwise 👍

core-bundle/src/Resources/contao/forms/FormRange.php Outdated Show resolved Hide resolved
ausi
ausi previously approved these changes Dec 25, 2019
aschempp
aschempp previously approved these changes Jan 2, 2020
@fritzmg fritzmg dismissed stale reviews from aschempp and ausi via 48fb5a5 January 2, 2020 13:35
Toflar
Toflar previously approved these changes Jan 2, 2020
@leofeyer
Copy link
Member

leofeyer commented Jan 2, 2020

Contao 4.9 is now in the review phase in which we try to review and merge the remaining pull requests that have been created during the development phase. Your pull request does not yet meet the requirements to be reviewed though.

  • Make sure that the PR is feature complete.
  • Rebase the PR onto the latest master branch.
  • Address the requested changes if any.
  • Make sure that all CI checks are successful.

Please fix this soon if you can, because the review phase ends on January 15th and we need some time to review the PR before we can merge it.

@fritzmg
Copy link
Contributor Author

fritzmg commented Jan 3, 2020

👍 I didn't know about the <output> element actually. @leofeyer wdyt? Browser support is fairly limited on Microsoft's end though.

@asaage
Copy link

asaage commented Jan 3, 2020

I think it could be any other element and the question is do we want a oninput -handler there.

@Toflar
Copy link
Member

Toflar commented Jan 3, 2020

That's just Javascript. It's not native browser behaviour so I don't think we should support it. We might want to add support for the data-list once it's unified across all browsers though.

@asaage
Copy link

asaage commented Jan 3, 2020

We might want to add support for the data-list

But then not seeing what you slide-to gets even worse 🤪

@leofeyer
Copy link
Member

leofeyer commented Jan 3, 2020

That's just Javascript. It's not native browser behaviour so I don't think we should support it.

I am with @Toflar here.

@fritzmg
Copy link
Contributor Author

fritzmg commented Jan 4, 2020

While I agree with adding the step attribute to the text field, I fail to see why we need a separate FormRange class. Its code is almost identical to the FormTextField class and we could easily implement the range type there by adding a new input validation.

Fine with me, I can change the PR accordingly :)

Still not sure if this is the right way to go. A new input validation would have the exact same settings as the digit validation.

Also setting the form type to "Text field" in order to get an <input type="range"> seems counter intuitive from a user experience perspective.

@Toflar
Copy link
Member

Toflar commented Jan 4, 2020

I agree, it should be its own widget (might extend or share logic though). Especially if the data-list options become standardized it will be interesting.

@fritzmg
Copy link
Contributor Author

fritzmg commented Jan 5, 2020

I made a few more changes:

It is perfectly valid to create form fields like this:

<input type="number" min="0.5" max="1.5" step="0.1" value="0.2">
<input type="range" min="0.5" max="1.5" step="0.1" value="0.2">

Thus I have changed the rgxp of the minval, maxval and step fields accordingly. I also changed the SQL definition of step to allow storage of floats. However, I am not sure if varchar(10) is the best choice here (that's what minval and maxval previously used also).

HTML output like this

<input type="range" value="">

is invalid, as the content of the value attribute must not be empty and it must be numeric, when defined. Thus I also changed the output to cast it to a float, instead of encoding any entities.

@leofeyer
Copy link
Member

leofeyer commented Jan 5, 2020

Also setting the form type to "Text field" in order to get an <input type="range"> seems counter intuitive from a user experience perspective.

The same is true for <input type="numeric">, isn't it? So either we have dedicated classes for all HTML5 input types or we integrate them all into the text field. But please not a mix of both.

@fritzmg
Copy link
Contributor Author

fritzmg commented Jan 5, 2020

But there is a clear difference between

  • <input type="date">
  • <input type="datetime-local">
  • <input type="email">
  • <input type="month">
  • <input type="number">
  • <input type="password">
  • <input type="search">
  • <input type="tel">
  • <input type="text">
  • <input type="time">
  • <input type="url">
  • <input type="week">

and

  • <input type="range">

The former input types all have a text field, where you click and then can enter values manually. The latter is handled completely differently from the others. Sure, some of the input types in the former list have additional client side validation and some added fluff (like spinners, datepickers, etc.). But their concept is similar to one another - while the range slider is a completely different UI concept.

(Sorry for the notification spam, my browser had some issues.)

@leofeyer
Copy link
Member

leofeyer commented Jan 6, 2020

Ok, that's a good point. 👍

@leofeyer leofeyer merged commit 101742f into contao:master Jan 8, 2020
@leofeyer
Copy link
Member

leofeyer commented Jan 8, 2020

Thank you @fritzmg.

@leofeyer leofeyer changed the title Add a range field and support step in text field Add a range field and support steps in text fields Jan 10, 2020
@fritzmg fritzmg deleted the feature/range branch October 24, 2021 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants