Skip to content

Cell Tooltip - Focus Issues #1392

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

Closed
wants to merge 5 commits into from

Conversation

JamesPortelli
Copy link
Contributor

  • Fix issues between cell action and cell tooltips whereby when trying to click on a cell-action the tooltip will show making it very difficult to select the options.

  • Remove un-referenced editors.

  • Clean up Frozen column code to get rid of z-index css.

Copy link
Contributor

@amanmahajan7 amanmahajan7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to use a portal for tooltips to prevent overlapping issues. Check the comments for more details


const tooltip = this.props.tooltip ? (<span className="cell-tooltip-text">{this.props.tooltip}</span>) : null;

const classes = joinClasses('react-grid-Cell__value',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to move the tooltip outside the grid otherwise there will be issues similar to editors. Currently if the cell has tooltip then a new div is rendered and positioned. I propose the following solution instead

return (
      <div
        {...this.getKnownDivProps()}
        className={className}
        style={style}
        {...events}
        ref={this.setCellRef}
      >
        {cellActionButtons}
        {cellExpander}
        { tooltip ? (<TooltipTrigger placement="left" overlay={tooltip}> {cellContent}<TooltipTrigger > : cellContent }
      </div>
    );

TooltipTrigger can render the tooltip in a portal and position it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of doing something along these lines as i too agree the tooltip should be rendered outside the cell.

My biggest reservation about going down this would is around having to attach a onMouseOver/Out event to each cell to trigger the showing and hiding of the tooltip, and as a result was concerned about its performance.

Also although we have tooltip components in AdazzleUI and one-strata-ui we cannot utilize them in this repository, so i would have to build one and I think having three different tooltip components can become unmaintainable

and to avoid having the tooltip hide under the cell in
the next row we need to up the z-index of the cell being hovered.
*/
z-index: 101;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can get rid of zIndex related code if we use the above mentioned solution

@amanmahajan7 amanmahajan7 changed the base branch from master to v5-patch December 5, 2018 15:29
@amanmahajan7 amanmahajan7 changed the base branch from v5-patch to master December 5, 2018 15:29
@amanmahajan7 amanmahajan7 mentioned this pull request Dec 5, 2018
3 tasks
@nstepien nstepien deleted the ja-tooltip-cell-action-focus-fix branch March 14, 2019 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants