feat(Slider): add experimental slider #1683

emyarod · 2018-12-21T00:58:40Z

Closes IBM/carbon-components-react#1682

This PR will add the new experimental slider changes

related https://github.com/IBM/carbon-components-react/issues/1681

netlify · 2018-12-21T01:00:53Z

Deploy preview for carbon-components-react ready!

Built with commit 6d88eed

https://deploy-preview-1683--carbon-components-react.netlify.com

asudoh · 2018-12-22T06:02:06Z

src/components/Slider/Slider.js

@@ -154,12 +153,6 @@ export default class Slider extends PureComponent {
    };
  }

-  UNSAFE_componentWillReceiveProps(nextProps) {
-    if (!sliderValuePropSync && !isEqual(nextProps, this.props)) {


The removal here seems a breaking change, please see: #1370

asudoh

Great progress @emyarod! Great to get in sync with TJ wrt the markup change.

asudoh · 2018-12-22T06:04:28Z

src/components/Slider/Slider.js

          <span className={`${prefix}--slider__range-label`}>
            {formatLabel(max, maxLabel)}
          </span>
          {!hideTextInput && (
            <TextInput
              type={inputType}
-              id="input-for-slider"
+              id={`input-for-slider${id}`}


We tend to use ${id} as a prefix instead of a suffix (e.g. https://github.com/IBM/carbon-components-react/blob/v6.39.0/src/components/Select/Select.js#L31) - Wondering if it'd make sense to align the convention.

asudoh

LGTM 👍 if the change works with v9 style. Do you think this is still blocked by #1681? Thanks @emyarod !

emyarod · 2019-01-08T14:49:09Z

@asudoh probably is not blocked but the topic discussed in #1681 is still something to consider in the future I believe

emyarod · 2019-01-09T15:57:54Z

@asudoh the visual bug we are seeing in the React implementation is caused by the problem addressed in https://github.com/IBM/carbon-components-react/issues/1681 and #1684

Our React <TextInput> includes a form item/<FormItem> wrapper which affects the styles that should be applied to the input field

so we end up rendering

<div class="bx--form-item">
  <input>
</div>

rather than just

<input>

asudoh · 2019-01-09T21:21:30Z

@emyarod As we discussed the slider element order issue is caused by https://github.com/IBM/carbon-components/pull/1315/files#diff-f8f1a0577791a85d255a9cdf4e2f791fR30 - Did you see it differently, or were you talking about another issue? Any more background?

emyarod · 2019-01-10T18:07:45Z

@asudoh from what I can tell, the issue is caused by our <TextInput> component's wrapper div.

https://github.com/IBM/carbon-components-react/blob/master/src/components/Slider/Slider.js#L464

https://github.com/IBM/carbon-components-react/blob/master/src/components/TextInput/TextInput.js#L77

removing this wrapper causes the input box to be positioned correctly

I believe this line is also relevant in why a wrapper div would ruin the order of the elements https://github.com/IBM/carbon-components/pull/1315/files#diff-f8f1a0577791a85d255a9cdf4e2f791fR38

asudoh · 2019-01-10T20:47:37Z

Thank you for the details @emyarod! I think the last link explains the issue the best. That said, when you remove the wrapper div, would you try adding display:flex to the new parent node in DOM inspector and see what happens?

emyarod · 2019-01-11T19:28:14Z

@asudoh it looks like the new parent node already contains display: flex after removing the wrapper div, since the new parent node is .bx--slider-container

asudoh · 2019-01-12T00:43:35Z

@emyarod Thank you for taking a look - Further investigation revealed that I was confused div.bx--form-item around text input vs. div.bx--form-item around slider, my apologies.

From that, I see two approaches to fix the problem:

Direct usage of <input> instead of using <TextInput>
Adding a CSS class for ordering <TextInput> within div.bx--form-item around slider

Thoughts...?

emyarod · 2019-01-14T16:31:32Z

@asudoh I am leaning towards option 2 right now, but eventually I would like to address this in a future release after reopening the discussion on #1684

emyarod · 2019-01-16T21:23:12Z

blocked by carbon-design-system/carbon#1640

asudoh

LGTM 👍 - Thanks @emyarod!

This reverts commit cc0c674.

This reverts commit f15b494.

emyarod · 2019-01-22T17:50:00Z

updated to include changes from carbon-design-system/carbon#1640

carbon-bot · 2019-01-22T23:36:26Z

🎉 This PR is included in version 6.81.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

emyarod added the status: blocked 🙅‍♀️ label Dec 21, 2018

asudoh reviewed Dec 22, 2018

View reviewed changes

emyarod force-pushed the 1682-experimental-slider branch 2 times, most recently from d09f186 to 37f63fa Compare January 2, 2019 19:20

emyarod force-pushed the 1682-experimental-slider branch from 37f63fa to 16cb84a Compare January 7, 2019 16:36

emyarod changed the title ~~feat(Slider): add experimental slider~~ [WIP] feat(Slider): add experimental slider Jan 7, 2019

emyarod force-pushed the 1682-experimental-slider branch from c18724d to aff2cd8 Compare January 7, 2019 21:17

asudoh reviewed Jan 8, 2019

View reviewed changes

emyarod added status: WIP 👩‍💻 and removed status: blocked 🙅‍♀️ labels Jan 8, 2019

emyarod force-pushed the 1682-experimental-slider branch from aff2cd8 to 67f271d Compare January 9, 2019 15:58

emyarod added the status: blocked 🙅‍♀️ label Jan 9, 2019

emyarod mentioned this pull request Jan 9, 2019

fix(slider): remove unused and unclosed div carbon-design-system/carbon#1609

Merged

emyarod force-pushed the 1682-experimental-slider branch from 67f271d to bd70265 Compare January 11, 2019 19:25

emyarod force-pushed the 1682-experimental-slider branch from bd70265 to 39880cf Compare January 14, 2019 16:30

This was referenced Jan 15, 2019

Favor markup rearrangement over order CSS property in experimental slider carbon-design-system/carbon#1639

Closed

refactor(slider): rearrange experimental markup instead of CSS order carbon-design-system/carbon#1640

Merged

emyarod changed the title ~~[WIP] feat(Slider): add experimental slider~~ feat(Slider): add experimental slider Jan 16, 2019

emyarod removed the status: WIP 👩‍💻 label Jan 16, 2019

emyarod force-pushed the 1682-experimental-slider branch 2 times, most recently from 4184c4e to 9c97e21 Compare January 21, 2019 15:57

asudoh approved these changes Jan 22, 2019

View reviewed changes

asudoh added status: one more review 👀 status: ready for review 👀 and removed status: blocked 🙅‍♀️ labels Jan 22, 2019

emyarod added 12 commits January 22, 2019 11:37

feat(Slider): match experimental markup

d4502c0

refactor(Slider): remove uncalled lifecycle method

b82f5b8

fix(Slider): declare correct prop types

176caaa

fix(Slider): use correct class name

1329396

fix(Slider): avoid duplicate IDs

f4f3c18

docs(Slider): set default input type

70789a8

Revert "refactor(Slider): remove uncalled lifecycle method"

4312147

This reverts commit cc0c674.

Revert "fix(Slider): avoid duplicate IDs"

7bf4da3

This reverts commit f15b494.

fix(Slider): avoid duplicate IDs

552fc9c

fix(Slider): add missing className

ae24938

chore: bump carbon-components version

ecdcea0

fix(Slider): match vanilla markup

6d88eed

emyarod force-pushed the 1682-experimental-slider branch from 9c97e21 to 6d88eed Compare January 22, 2019 17:48

joshblack approved these changes Jan 22, 2019

View reviewed changes

asudoh merged commit 5d4b040 into carbon-design-system:master Jan 22, 2019

carbon-bot added the released label Jan 22, 2019

emyarod deleted the 1682-experimental-slider branch January 23, 2019 16:19

emyarod mentioned this pull request Apr 30, 2019

Form spacing inconsistencies carbon-design-system/carbon#2343

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Slider): add experimental slider #1683

feat(Slider): add experimental slider #1683

emyarod commented Dec 21, 2018 •

edited

Loading

netlify bot commented Dec 21, 2018 •

edited

Loading

asudoh Dec 22, 2018

asudoh left a comment

asudoh Dec 22, 2018

asudoh left a comment

emyarod commented Jan 8, 2019

emyarod commented Jan 9, 2019

asudoh commented Jan 9, 2019

emyarod commented Jan 10, 2019 •

edited

Loading

asudoh commented Jan 10, 2019

emyarod commented Jan 11, 2019

asudoh commented Jan 12, 2019

emyarod commented Jan 14, 2019

emyarod commented Jan 16, 2019

asudoh left a comment

emyarod commented Jan 22, 2019

carbon-bot commented Jan 22, 2019

feat(Slider): add experimental slider #1683

feat(Slider): add experimental slider #1683

Conversation

emyarod commented Dec 21, 2018 • edited Loading

netlify bot commented Dec 21, 2018 • edited Loading

asudoh Dec 22, 2018

Choose a reason for hiding this comment

asudoh left a comment

Choose a reason for hiding this comment

asudoh Dec 22, 2018

Choose a reason for hiding this comment

asudoh left a comment

Choose a reason for hiding this comment

emyarod commented Jan 8, 2019

emyarod commented Jan 9, 2019

asudoh commented Jan 9, 2019

emyarod commented Jan 10, 2019 • edited Loading

asudoh commented Jan 10, 2019

emyarod commented Jan 11, 2019

asudoh commented Jan 12, 2019

emyarod commented Jan 14, 2019

emyarod commented Jan 16, 2019

asudoh left a comment

Choose a reason for hiding this comment

emyarod commented Jan 22, 2019

carbon-bot commented Jan 22, 2019

emyarod commented Dec 21, 2018 •

edited

Loading

netlify bot commented Dec 21, 2018 •

edited

Loading

emyarod commented Jan 10, 2019 •

edited

Loading