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

[WIP] Implement Slider #44

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

motiz88
Copy link
Collaborator

@motiz88 motiz88 commented May 28, 2018

Hey there, just wanted to share this early implementation of <Slider> based on <input type="range">. I've not begun to tackle styling in any way and there are probably a dozen other things that need to change before this can be considered complete. I'm doing this to learn, so any and all feedback would be appreciated.

Status and next steps

  • Create rndom-slider Svelte component.
    • Create Flow libdef for rndom-slider.
  • Port Libraries/Components/Slider/Slider.js as Slider.dom.js - specifically from this version to stay in sync with the rest of RNDom and particularly RNTester.
  • Set an explicit default height (currently 40 to match iOS).
  • Create RCTSlider and wire it up with rndom-slider.
  • Create and register RCTSliderManager and wire it up with RCTSlider.
  • Implement value, minimumValue, maximumValue, step, disabled, onValueChange, onSlidingComplete.
  • Implement or remove colour styling props:
  • Implement or remove (probably remove?) thumbImage, trackImage, minimumTrackImage, maximumTrackImage.
    • If implementing, patch RNTester to enable the corresponding examples.
  • Apply a consistent cross-browser visual style a la Bootstrap (also as a workaround for this Chrome bug).

@vincentriemer
Copy link
Owner

So awesome! I found this article about styling sliders, looks like cross browser support makes it tricky but it's possible.

!rntester

@motiz88
Copy link
Collaborator Author

motiz88 commented May 28, 2018

There's a followup to that article, entitled A Sliding Nightmare: Understanding the Range Input, and... yes. Yes it is.

I seem to be hitting https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/14489843/, so I'll clean up and commit my current work on styling and thumbTintColor with the caveat that it's quite borked on Edge (but works reasonably well across Chrome, Firefox & Safari).

@motiz88
Copy link
Collaborator Author

motiz88 commented May 28, 2018

My visual styling approach so far in this branch has been:

  1. Rip out relevant code and defaults from Bootstrap's _custom-forms.scss.
  2. Convert all sizes from rem to px using 16px = 1rem as a base.
  3. Tweak the track and thumb sizes to taste (very subjective ofc).
  4. Heavily condense the repetitive parts with mixins, and inline variables/mixins that are only used once.
  5. Convert the code from SCSS to LESS (personal preference due to LESS having a pure JS compiler and hence a smoother install process).
  6. Add a LESS preprocessing step to the Svelte build config in rndom-slider.
  7. Use CSS custom properties to link thumbTintColor into the actual styles.
  8. Use Qix-/color to recreate in JS what Bootstrap was doing with SCSS/LESS colour functions for shading and focus effects, since compile-time colour functions can't operate on var() references and there is no pure CSS equivalent that can.

@motiz88
Copy link
Collaborator Author

motiz88 commented May 28, 2018

Do I have !rntester privileges? 😋 Oh well, it was worth a shot.

@motiz88
Copy link
Collaborator Author

motiz88 commented May 28, 2018

The two major problems at the moment are:

  • The limitations of a CSS custom properties approach - most annoyingly the Edge bug referenced above. I think browser support is otherwise decent enough for current goals, but need some maintainer steer on this.

  • Subtle inconsistencies between browsers with regard to rendering and interaction.

In light of these issues, it may be worth reverting to a plain DOM implementation (or an existing 3rd party lib) that just does its own rendering, event handling, accessibility, etc, rather than try to tame <input type="range">.

@vincentriemer Thoughts? What would be your criteria for merging this feature?

@vincentriemer
Copy link
Owner

I think that a plain DOM approach may be best, I have avoided using CSS Custom Properties due to wonky support in Firefox/Edge. You can find a good example of the accessibility considerations here and just as importantly, ensure it behaves as a user would expect it to when using it on a touch device.

!rntester

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.

2 participants