-
Notifications
You must be signed in to change notification settings - Fork 45
Tooltip component created #9
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
Conversation
@ssi114 Can you check why CI build is failing and fix it? |
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.
Doesn't take care of web accessibility:
https://www.w3.org/TR/wai-aria-practices-1.1/#tooltip
https://www.sarasoueidan.com/blog/accessible-tooltips/
<span className={className} onMouseLeave={this.hideTooltip}> | ||
{displayTooltip && ( | ||
<div className={classNames('tooltip-bubble', position)}> | ||
<div className="tooltip-message">{message}</div> |
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.
@ssi114 i would expect tool tip to get triggered from existing components like 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.
@ssi114 Please fix these review comments.
|
||
class Tooltip extends Component<Props, State> { | ||
static defaultProps = { | ||
message: '', |
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 Props will not be string.
this.setState({ displayTooltip: true }); | ||
}; | ||
|
||
render() { |
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.
You should use react portal so that tooltip doesn't get cropped because of overflow hidden on parent.
TooltipComponent = shallow(<Tooltip>Test</Tooltip>); | ||
}); | ||
|
||
test('should render correctly', () => { |
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 test will not be 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.
There are still lot of things to fix @ssi114
import styles from './Tooltip.style'; | ||
import type { Props, State } from './types'; | ||
|
||
class Tooltip extends Component<Props, State> { |
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.
@ssi114 this component can be PureComponent and not a Component
}; | ||
|
||
componentDidMount() { | ||
document.addEventListener('keyup', (e: KeyboardEvent) => { |
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.
keyup event should change to keydown and event type could be KeyboardEvent use SyntheticEvent instead it will cover both Mouse and Keyboard
</div> | ||
)} | ||
<span className="tooltip-trigger" role={ariaRole}> | ||
{children} |
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.
Children should be body of tooltip and not the trigger
@vinodloha I am picking up this task |
No description provided.