-
Notifications
You must be signed in to change notification settings - Fork 166
Conversation
packages/terra-switch/src/terra-dev-site/doc/switch/UpgradeGuide.2.doc.mdx
Outdated
Show resolved
Hide resolved
packages/terra-switch/src/Switch.jsx
Outdated
|
||
delete customProps.className; | ||
return ( | ||
<button |
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.
Set an aria-label containing the labelText on the root button element.
packages/terra-switch/src/Switch.jsx
Outdated
|
||
delete customProps.className; | ||
return ( | ||
<button |
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.
Set <button>
to <div>
, this will resolve the focus issue with the preventDefault removed and associated css change.
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.
Are you referring to prevent default of mouseDown Event..?
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.
changed button to div : 22dedf6f . but we need preventDefault on both handleKeyDown and handleMouseDown to keep switch focus styles consistent in IE.
}; | ||
|
||
const handleOnMouseDown = (event) => { | ||
event.preventDefault(); |
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.
Remove prevent default on mouseDown.
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.
I tried removing this. but if we remove focus styles starts appear for mouse interactions in IE.
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.
Just checked, yeah you are correct, that part combined with the focus is correct. Disregard this comment.
// See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Button#Clicking_and_focus | ||
// Button on Firefox, Safari and IE running on OS X does not receive focus when clicked. | ||
// This will set focus on the button when clicked. | ||
sliderButton.current.focus(); |
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.
No longer needed with associated div change.
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.
But after removing sliderButton.current.focus();
focus styles were not getting displayed in IE.
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.
Just checked, yeah you are correct IE 10 has issues.
.switch { | ||
background-color: transparent; | ||
border: 0; | ||
display: flex; |
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.
Update to display: inline-flex
.
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.
updated to inline-flex
} | ||
|
||
// Removes dotted border of button on focus that shows up in Firefox | ||
&::-moz-focus-inner { |
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.
Changing the button to a div will remove the need for this pseudo selector.
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.
packages/terra-switch/src/Switch.jsx
Outdated
disabled={isDisabled} | ||
aria-checked={isChecked} | ||
role="switch" | ||
tabIndex="0" |
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.
If isDisabled
is set the following attribute should not be set on the switch:
tabIndex="0"
onBlur={handleOnBlur}
onClick={handleOnClick}
onMouseDown={handleOnMouseDown}
onKeyDown={handleOnKeyDown}
Can instead add some dependent logic like:
let switchAttrs;
if (!isDisabled) {
switchAttrs = {
tabIndex: '0',
onBlur: handleOnBlur,
onClick: handleOnClick,
onMouseDown: handleOnMouseDown,
onKeyDown: handleOnKeyDown,
};
}
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.
added attributes variable
packages/terra-switch/src/Switch.jsx
Outdated
|
||
const labelClassNames = cx([ | ||
'label-text', | ||
{ 'is-disabled': isDisabled }, |
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.
Now that there is a single entry point for the control we can remove the individual is-disabled
classes from the sub-elements and instead with child/nested selectors from the parent tray's is-disabled
class.
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.
but Since now we have changed button to div disabled is not happening as it was happening before when button was wrapped around all other controls. sub-elements do have theme variables for disabled styles like tray has theme variables for background-color, box-shadow and border styles. So removing these classes will enable focus styles of slider on TAB key press and hover styles on mouse hover, even when switch is disabled.
border: 0; | ||
outline: 0; | ||
|
||
&[data-terra-switch-show-focus-styles='true'] .slider:not(.is-disabled) { |
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.
If focus is no longer possible with the isDisabled
check in the javascript, we can remove this qualifiers for not(.is-disabled)
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.
this we can remove only whole component was just a button but now after changing this to div we still need to prevent focus styles for disabled switch.
@supreethmr Here is a branch for reference to the proposed changes for the disabled behavior. There are some ways to reduce the css further, but I didn't want to mess with any of the variables. |
Thank you for creating branch for code changes. I have updated the PR with suggested changes : 7a753a7 |
With Dave's approval, I'm good with this now.
I'm still good with this refactor. |
Functional review complete. No issues identified. |
Summary
Resolves #2520
Switch component represents toggle button that indicates whether a setting is on or off and provide users with instantaneous feedback.
Deployment:
https://terra-core-deployed-pr-2846.herokuapp.com/components/terra-switch/switch/switch
Requirements:
zeplin.
Bidirectionality:
Alignment will be mirrored in rtl orientation.
Accessibility:
Component will be keyboard navigatable
React Props: