Add SLDS Slider #1317

dlferro · 2018-04-02T01:12:13Z

This adds the SLDS Slider component to the component library.

Fixes #1314

@interactivellama

Pull Request Review checklist (do not remove)

Review the appropriate Storybook stories. Open http://localhost:9001/.
Review tests are passing in the browser. Open http://localhost:8001/.
Review markup conforms to SLDS by looking at snapshot strings.
Add year-first date and commit SHA to last-slds-markup-review in package.json and push.
Request a review of the deployed Heroku app by the Salesforce UX Accessibility Team.
Add year-first review date, and commit SHA, last-accessibility-review, to package.json and push.
While the contributor's branch is checked out, run npm run local-update within locally cloned site repo to confirm the site will function correctly at the next release.

Conflicts: package-lock.json

interactivellama

Awesome first time contribution!!! I left some notes on the code. npm run lint:fix should fix the lint issues.

interactivellama · 2018-04-04T00:59:38Z

components/slider/__examples__/sizes-large.jsx

+
+	render () {
+		return (
+      <Slider


I think it is a good idea for code examples to be spaces, so this is a good call, but please convert all to tabs for consistency.

interactivellama · 2018-04-04T01:01:23Z

components/slider/__examples__/error.jsx

+	}
+});
+
+export default Example;


Please remove min-max-step.jsx and min-max.jsx since they are in base.

interactivellama · 2018-04-04T01:06:49Z

components/slider/__examples__/sizes.jsx

+import Slider from '~/components/slider';
+
+const Example = createReactClass({
+	displayName: 'IconExample',


Please update displayName .

interactivellama · 2018-04-04T01:10:57Z

components/slider/index.jsx

+
+import { SLIDER } from '../../utilities/constants';
+
+const propTypes = {


Please sort alphabetically.

interactivellama · 2018-04-04T01:13:09Z

components/slider/index.jsx

+	 * Text that is visually hidden but read aloud by screenreaders to tell the user what the Slider is for.
+	 * If the Slider has a visible label, you can omit the <code>assistiveText</code> prop and use the <code>label</code> prop.
+	 */
+	assistiveText: PropTypes.object,


Please list out label key/value pair.

Needs to be PropTypes.shape so that it can be undefined.

interactivellama · 2018-04-04T01:35:58Z

components/slider/index.jsx

+ * The ability to style sliders with CSS varies across browsers. Using this component ensures sliders look the same everywhere.
+ */
+class Slider extends React.Component {
+	static displayName = SLIDER;


Please pull in from constants file like

import { ACCORDION } from '../../utilities/constants';

interactivellama · 2018-04-04T01:39:50Z

components/slider/index.jsx

+	}
+
+	handleChange = (event) => {
+		this.setState(({ value: event.target.value }));


I'm not sure we need state. The value should be the value directly from the props and not the DOM event, so this can be controlled. The examples will need a state container around them then though.

@interactivellama just to clarify, the user should be the one in charge of making sure that span.slds-slider__value is up to date with the current slider value(as per implementation note https://www.lightningdesignsystem.com/components/slider/)?

To be a controlled component, the user should be triggering an onChange event that passes the value up to a state container (or Redux in an app) that then passes it down as the props.value. You can see this in other examples, for instance, a controlled <Input/> textbox. The keyPress fires of the change event that then changes the value and loops it back in. This allows the component to be a functional stateless components.

interactivellama · 2018-04-04T01:41:06Z

components/slider/index.jsx

+							id={this.getId()}
+							name={this.props.name}
+							className="slds-slider__range"
+							value={this.state.value}


Please replace with props.value.

interactivellama · 2018-04-04T01:41:40Z

components/slider/index.jsx

+							className="slds-slider__value"
+							aria-hidden="true"
+						>
+							{this.state.value}


Please replace with props.value.

interactivellama · 2018-04-04T02:07:26Z

components/slider/index.jsx

+	 */
+	disabled: PropTypes.bool,
+	/**
+	 * Size of the slider.


We should mention this is only horizontal size.

- Updates to match implementation guidelines. - Added state component wrapper to show updating the value. - Removed unneeded documentation. - Added defaultValue prop.

dlferro · 2018-04-06T15:48:48Z

@interactivellama should be good. Let me know if I missed anything.

Resolved missing aria attribute in snapshot testing.

interactivellama · 2018-04-10T04:56:05Z

components/slider/index.jsx

 	/**
-	 * Message to display when the Slider is in an error state. When this is present, also visually highlights the component as in error.
+	 * This event fires whenever the user has modified the data of the control.


* This event fires whenever the user has modified the data of the control. event, { value: [string] } is passed in.

~~See https://github.com/salesforce/design-system-react/blob/master/components/combobox/combobox.jsx#L427~~

Oh, sorry. That's exactly what you did. The tests though seem to not get an object, but a straight number. The test probably just needs updating.

@interactivellama can you clarify which prop you're referring to?

Updated to follow the combobox implementation, passing an object with key of value.

salesforce-cla · 2020-08-06T17:35:59Z

Thanks for the contribution! Before we can merge this, we need @dlferro to sign the Salesforce.com Contributor License Agreement.

dlferro added 2 commits April 1, 2018 20:54

Added slds slider component.

f7f2cfd

Merge remote-tracking branch 'refs/remotes/remote/master'

896844c

Conflicts: package-lock.json

salesforce-cla bot added the cla:signed label Apr 2, 2018

dlferro changed the title ~~Slider~~ Added SLDS Slider Apr 2, 2018

interactivellama self-requested a review April 2, 2018 19:29

interactivellama requested changes Apr 4, 2018

View reviewed changes

9529d9a

- Updates to match implementation guidelines. - Added state component wrapper to show updating the value. - Removed unneeded documentation. - Added defaultValue prop.

dlferro added 2 commits April 9, 2018 09:57

Updated snapshot.

e732ad6

510994a

Resolved missing aria attribute in snapshot testing.

interactivellama reviewed Apr 10, 2018

View reviewed changes

interactivellama temporarily deployed to design-system-react-co-pr-1317 April 10, 2018 17:30 Inactive

Updated event to be consistent in passing an object for second parameter

3464af7

interactivellama deployed to design-system-react-co-pr-1317 April 10, 2018 18:05 View deployment

interactivellama approved these changes Apr 11, 2018

View reviewed changes

interactivellama merged commit 3a4f467 into salesforce:master Apr 11, 2018

interactivellama changed the title ~~Added SLDS Slider~~ Add SLDS Slider Apr 17, 2018

dlferro deleted the slider branch June 19, 2018 23:27

salesforce-cla bot removed the cla:signed label Aug 6, 2020

salesforce-cla bot added cla:missing cla:signed and removed cla:missing labels Aug 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SLDS Slider #1317

Add SLDS Slider #1317

dlferro commented Apr 2, 2018 •

edited by interactivellama

Loading

interactivellama left a comment

interactivellama Apr 4, 2018

interactivellama Apr 4, 2018

interactivellama Apr 4, 2018

interactivellama Apr 4, 2018

interactivellama Apr 4, 2018

interactivellama Apr 4, 2018

interactivellama Apr 4, 2018

interactivellama Apr 4, 2018

dlferro Apr 4, 2018

interactivellama Apr 5, 2018

interactivellama Apr 4, 2018

interactivellama Apr 4, 2018

interactivellama Apr 4, 2018

dlferro commented Apr 6, 2018

interactivellama Apr 10, 2018 •

edited

Loading

interactivellama Apr 10, 2018

dlferro Apr 10, 2018

dlferro Apr 10, 2018

salesforce-cla bot commented Aug 6, 2020


		import { SLIDER } from '../../utilities/constants';

		const propTypes = {

Add SLDS Slider #1317

Add SLDS Slider #1317

Conversation

dlferro commented Apr 2, 2018 • edited by interactivellama Loading

Pull Request Review checklist (do not remove)

interactivellama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlferro commented Apr 6, 2018

interactivellama Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salesforce-cla bot commented Aug 6, 2020

dlferro commented Apr 2, 2018 •

edited by interactivellama

Loading

interactivellama Apr 10, 2018 •

edited

Loading