-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
modified: demos/index.html new file: demos/range-slider.html modified: packages/material-components-web/material-components-web.scss new file: packages/mdc-range-slider/README.md new file: packages/mdc-range-slider/_mixins.scss new file: packages/mdc-range-slider/_variables.scss new file: packages/mdc-range-slider/mdc-range-slider.scss new file: packages/mdc-range-slider/package.json modified: webpack.config.js
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
who's robot? |
CLAs look good, thanks! |
@import "variables"; | ||
@import "mixins"; | ||
|
||
[type="range"].mdc-range-slider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't really need the type here. Not only does it break BEM (which you also should add a linting marker for) but it isn't really necessary. The docs will instruct people to use this against a range type only and that should be sufficient. This is increasing specificity which makes it more difficult for developers to modify styling if they wish to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point,
As I recall from MDL 1.x feedback we were planning on not relying on the range input type with MCW due to some issues getting the MD styling just right. @sgomes Do you recall what those issues were? If not I can go search the issue tracker. |
@Garbee is it an issue with webkit having only one pseudo element for the whole track? |
Thanks, @MarcMcIntosh, this is a lot of work! As @Garbee was mentioning though, I fear we'll quickly run into issues if we try to implement things on top of native controls. Here are some issues I remember off the top of my head:
As such, we had decided against using the native control in MDC-Web. @traviskaufman Any insights from your non-native prototype? |
Thanks @sgomes I was making one figured may as well add it :) didn't even think about touch issues... , ran into a few of those inconsistencies. It probally have been easier to just to write inline an svg with min and max values applied to the view box. |
modified: packages/mdc-range-slider/mdc-range-slider.scss
Hey @MarcMcIntosh First off, thanks for taking the time to put all of this together, I really appreciate your willingness to contribute to our codebase! That being said, all of @sgomes' points are valid. Besides just that, MD sliders require some very intricate treatments that are not present in the spec, and that most (if not all?) existing MD implementations haven't nailed down properly. Thus, I'm probably going to have to close this PR for now, since I don't think we'll be able to land it in our codebase. That being said, we're tracking the slider issue at #25. If this is a component you're interested in undertaking, we'd be happy to help you out but ask that you follow the processes outlined within our authoring components doc. Finally, if you'd rather release this as a standalone component built on top of MDC-Web, that's totally doable as well! You can simply depend on |
@traviskaufman I was using the SCSS code base anyway and need a slider for a small project and figured may as-well make one. I've got a couple of projects I'd like to finish before making additional commitments. Shouldn't take to long though so I've taken a look at the react Checkbox.js in the framework-examples to get an idea of how javascript components are intended to be used.... Might take a while to get my head around guessing user provides the elements id's and class names... mdc attaches listeners and classname modifiers to them? |
Had to use slightly different implementations for gecko, webkit and edge.
I've tested on chromium and firefox, firefox looks the best as chromium has a very limited selection of pseudo elements for styling ranged inputs.
Kind of working blind with edge / internet explorer,
if any one could check that out it would be great :)