-
Notifications
You must be signed in to change notification settings - Fork 372
fix(DualList): Add item disability functionality. #1071
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
fix(DualList): Add item disability functionality. #1071
Conversation
PatternFly-React preview: https://1071-pr-patternfly-react-patternfly.surge.sh |
This comment has been minimized.
This comment has been minimized.
a783e00
to
fdbb08d
Compare
hidden: false | ||
hidden: false, | ||
disabled: false, | ||
tooltipID: `dual-list-item-tooltip-${UUID()}`, |
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 only runs once at module import. Not when the component is instantiated so if someone mounts multiple of these they will all share the same id.
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.
Wow thanks.. I didn't know that..
maybe set the ID by the item side+position which is unique enough
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.
What we've done in other components is to set the default to null then use it like:
const randomId = () => Date.now();
...
<Tooltip id={tooltipID || randomId()}>
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.
Tell me what do you think, added this:
const getTooltipID = () => {
let uniqueTooltipID = `dual-list-item-tooltip-${side}`;
if (parentPosition) {
uniqueTooltipID += `-${parentPosition}`;
}
uniqueTooltipID += `-${position}`;
return uniqueTooltipID;
};
and then using it like this when the default tooltipID
is null
.
<DualListItemTooltip text={tooltipText} id={tooltipID || getTooltipID()}>
fdbb08d
to
0086e3a
Compare
@@ -0,0 +1,30 @@ | |||
import React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import UUID from 'uuid/v1'; |
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'd prefer not to pull this in and just use the Data.now() function for a unique ID.
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.
oops, forgot to remove this one..
what do you think of the implementation above? isn't it unique enough ?
thanks
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.
Yup. Thanks.
0086e3a
to
8638362
Compare
&.disabled { | ||
cursor: not-allowed; | ||
background: @color-pf-black-150; | ||
color: @color-pf-black-500; |
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.
nit. supposed to be in alphabetical order.
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 haven't been keeping up with that in this repo
When an item is disabled,

the user won't be able to move it between the lists: