Skip to content
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

Tooltip #128

Merged
merged 13 commits into from
Feb 13, 2019
Merged

Tooltip #128

merged 13 commits into from
Feb 13, 2019

Conversation

lederer
Copy link
Contributor

@lederer lederer commented Jan 22, 2019

Overview

Add a Tooltip component.

Per #28, this component relies on popper.js (more specifically tooltip.js, which relies on popper.js).

Most React libraries that provide support for adornments like tooltips take a HOC approach or require inverted syntax like:

<Tooltip message="Ahoy there!">
    <ComponentToBeAdornedWithTooltip />
</Tooltip>

I struggle conceptually with those approaches. Adornments seem like they ought to be subordinately attached to their reference components somehow, rather than wrapped around them. So I decided to author our Tooltip component to be nested within its reference component. This solution, at least in its current state, has its shortcomings, but is functional and I think worth trying out.

Usage

<ComponentToBeAdornedWithTooltip data-tooltip>
    ...
    <Tooltip>Ahoy there!</Tooltip>
</ComponentToBeAdornedWithTooltip>

Requirements

  • Must add data-tooltip attribute to reference component.
  • Must be nested inside its reference component
  • Consequently, it cannot be used on components that represent singleton elements (eg, <img>, <input>).
    • If a tooltip is necessary for such a component, wrap it in a Box.
  • Tooltip only supports a single child that must be a string.
    • Future work: Support arbitrary jsx children.

Component authoring

When authoring a component that ignores props.children (eg, Pill, Swatch) and might be used with a Tooltip, your must do something like the following or you'll ignore the Tooltip, too:

import { extractComponentFromChildren } from "../../utils";
...
const Foo = ({children, ...props}) => {
  const [ignoredChildren, tooltip] = extractComponentFromChildren(children, 'Tooltip');
  return (
    <StyledFoo {...props}>
      {tooltip}
    </StyledFoo>
  );
};

Checklist

  • Relevant documentation pages have been created or updated
  • Description of PR is in an appropriate section of the changelog and grouped with similar changes if possible

Are there any of the following in this PR?

  • Changes to the prop names or types of existing components
  • Changes in intended behavior of existing component props
  • Changes in the theme file's structure
  • A required version bump to a non-dev dependency of the project

If one of the above is checked...

  • information or guidance around needed changes for consumers of this library has been added to the changelog under Upgrade Instructions

Demo

image

Notes

I had to parameterize Tooltip.js somewhat to make it work as the basis of this component:

  • Custom html template to make the arrow a child of the tooltip content rather than a sibling. This is because our styled-component styling can only affect the tooltip content, and so to style the arrow, it must be a descendant thereof.
  • Some show/hide fiddling upon tooltip instantiation to ensure it renders in the correct position from the start.

I added a utils.js file with an extractComponentFromChildren() function. Not sure if I placed utils.js in the appropriate place in the src folder tree.

Future work might consider alternate tooltip libraries, eg:

Closes #28

@lederer
Copy link
Contributor Author

lederer commented Jan 22, 2019

Also did a drive-by breadcrumb alignment fix (#127).

@lederer lederer force-pushed the feature/sml/tooltip branch from 52e7b36 to 650b7ed Compare February 8, 2019 14:46
@lederer
Copy link
Contributor Author

lederer commented Feb 8, 2019

Rebased and moved Tooltip docz page into Overlays section.

Copy link
Contributor

@designmatty designmatty left a comment

Choose a reason for hiding this comment

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

This looks good and functions as I'd expect. Let's get this merged in once the conflicts are resolved.

There is a warning about invalid DOM nesting which I'd like to handle at some point in the future with another PR. It could have undesired outcomes in different browsers.

For example, currently, if you apply a tooltip in a button and have a button within that tooltip (see doc example), technically you have illegally nested buttons as far as browsers are concerned. But the way we ultimately render the component removes the nested structure in the DOM but only after the browser gives us a console warning. Since, at the end of the day, we aren't maintaining an illegal nesting structure, I propose we make an issue for this and fix it later.

@lederer
Copy link
Contributor Author

lederer commented Feb 13, 2019

There is a warning about invalid DOM nesting which I'd like to handle at some point in the future with another PR. It could have undesired outcomes in different browsers.

Darn. Thought I'd found a way around that. I may take one more look at that before merging.

@lederer lederer force-pushed the feature/sml/tooltip branch from 650b7ed to 4b519a1 Compare February 13, 2019 15:43
@lederer lederer merged commit 557e285 into master Feb 13, 2019
@lederer lederer deleted the feature/sml/tooltip branch February 13, 2019 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants