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

Improve range-field #1016

Merged
merged 8 commits into from
Mar 18, 2017
Merged

Improve range-field #1016

merged 8 commits into from
Mar 18, 2017

Conversation

OleVik
Copy link
Contributor

@OleVik OleVik commented Mar 12, 2017

Adds rangetouch.js (2.04 KB minified, pure JS), some SCSS (6.87 KB compiled minified, source), and changed Twig to improve the functionality and styling of the range-field. Notes:

  1. As remarked in #995, there is near-universal support for the input-types (input-range, input-number), but on native touch-devices behavior does not always meet expectations. This is helped by Rangetouch.js, which is added as a dependency in package.json.

  2. Most of the weight of the CSS (5.20 KB) is a generated box-shadow-property which enables the filled/unfilled appearance of the slider. CSS is vendor-prefixed for cross-browser compatibility. The entirety of SCSS is in themes/grav/scss/template/modules/_input-range.scss, imported at the end of themes/grav/scss/preset.scss. This was the simplest way of inheriting the theme's default variables for colors, but I'd happily restructure it for better organization. SCSS variables are prefixed with rangefield- and styles nested in a rangefield-class to avoid future conflicts.

  3. Width is the same as the colorpicker-field (230px), and number-input width is set to hold up to four integers. This ensures that the input-tags remain on the same line even on smaller resolutions. Thumb size is 2.125em x 2.125em, and slider size is 1.35em. If changed, note that the latter is sensitive to the former to match heights - depending on base font-size slider should be 34 to 36 percentage smaller than thumb.

  4. The range- and number-fields are synced with a small piece of oninput, using declared id-attributes where available or generated as a fallback. Added the step-attribute, and number-field value fallbacks. Attributes min, max, step are not supported on mobile for number-input, but they work on range-input.

Built files with default gulp task, let me know if I should do it some other way. Preview of field:

Chrome:
range_chrome

Firefox (by default their number-input steps are visible):
range_firefox

Edge:
range_edge

Adds some JS (2.04 KB), SCSS (6.87 KB), and changed Twig to improve the functionality and styling of the range-field.
@w00fz
Copy link
Member

w00fz commented Mar 12, 2017

I haven't tested it but on first review this looks good!
One request only, can you add the library into the vendor list in webpack an rerun gulp (https://github.com/getgrav/grav-plugin-admin/blob/develop/themes/grav/webpack.conf.js)?

This is so the vendor and the admin are kept separated.

Cheers

@OleVik
Copy link
Contributor Author

OleVik commented Mar 12, 2017

There, added rangetouch to webpack and ran gulp. Is that sufficient to keep vendor and admin separate? Only vendor.min.js changed.

@w00fz
Copy link
Member

w00fz commented Mar 13, 2017

That should be enough

@rhukster
Copy link
Member

@OleVik can you re-merge? compiled css is conflicting.. cheers.

@OleVik
Copy link
Contributor Author

OleVik commented Mar 14, 2017

That should do it, I think.

@rhukster
Copy link
Member

I'm having a little problem with this:

First i set images.default_image_quality option in the system.yaml blueprint to use the range field:

                images.default_image_quality:
                    type: range
                    label: PLUGIN_ADMIN.DEFAULT_IMAGE_QUALITY
                    help: PLUGIN_ADMIN.DEFAULT_IMAGE_QUALITY_HELP
                    validate:
                        min: 1
                        max: 100

Then I reloaded the page and tested. First it didn't quite render the same as your screenshots. When i tried to change the range with the numeric input, i got errors, and updating the slider caused the same thing:

2017-03-15 at 3 23 pm

Copy link
Member

@rhukster rhukster left a comment

Choose a reason for hiding this comment

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

Ole, please see my comments in the conversation. I can't get this to work. not sure if i'm doing something wrong or if the range field expects something i'm not using. It should not need an ID for example.

@OleVik
Copy link
Contributor Author

OleVik commented Mar 18, 2017

This is "simply" because images.default_image_quality would render as range_images.default_image_quality and number_images.default_image_quality, for the respective callbacks used in onInput. A quick |replace('.', '_') with Twig solves it for any nested fields declared in blueprint, though it's not as elegant. Incoming commit.

Replaces dots with underscores for id-attributes within the field.
@rhukster rhukster merged commit b0dff58 into getgrav:develop Mar 18, 2017
@rhukster
Copy link
Member

Ok I merged this but i did redo the CSS a little to make things a little cleaner looking (IMHO), and more consistent between browsers. I also put support back for field.append which is handy for this particular use case as it can provide some units.

Chrome
chrome

Firefox
firefox

Edge
edge

@OleVik
Copy link
Contributor Author

OleVik commented Mar 18, 2017

Looks good, are those appends still accessible as number-type inputs? One previous problem was that there was no granular control of the selected number.

@rhukster
Copy link
Member

its just a text input you can put after.. the counter/numeric is still a number field, just right aligned so it's more flush with the append (if provided)

@OleVik
Copy link
Contributor Author

OleVik commented Mar 18, 2017

Looking forward to using it and releasing new versions of plugins without the custom form. 1.3.0 looks to be just around the corner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants