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

Remove default styling for Tooltip #650

Closed
jgonera opened this issue Mar 11, 2020 · 4 comments · Fixed by #666
Closed

Remove default styling for Tooltip #650

jgonera opened this issue Mar 11, 2020 · 4 comments · Fixed by #666

Comments

@jgonera
Copy link

jgonera commented Mar 11, 2020

Would you be open to remove these default styles for Tooltip?

https://github.com/hshoff/vx/blob/ac76afad960c4715a1f339b395e5b1e6a4e7de9c/packages/vx-tooltip/src/tooltips/Tooltip.tsx#L25-L32

It seems like most of VX doesn't assume much styling, but for Tooltip I need to override all of these with !important in my CSS modules. I know one could just replace Tooltip with a div, but TooltipWithBounds is harder to replace.

@geekplux
Copy link
Contributor

Inline style should work, but what you mentioned is a problem exactly, using css files to add style might be better

@williaster
Copy link
Collaborator

This component is quite simple so I'm curious about the benefits of this versus creating your own component? if you are using withTooltip, you should be able to specify your own Tooltip component there.

Alternatively we could update the component such that these current styles are set by default:

export default function Tooltip({
  className,
  top,
  left,
  style = defaultStyles,
  children,
  ...restProps
}: TooltipProps & JSX.IntrinsicElements['div']) {
  return (
    <div
      className={cx('vx-tooltip-portal', className)}
      style={style}
      {...restProps}
    >
      {children}
    </div>
  );
}

I think in this case we should export the default styles so if you want to e.g., override 1 style, you don't have to define all of them and could do <Tooltip style={{ ...defaultStyles, color: 'magenta' }} ... />.

I don't have a ton of bandwidth right now and am focusing on improved docs, but would happily review a PR!

@dennisja
Copy link
Contributor

dennisja commented Apr 8, 2020

I can make a PR to do this, What do you guys think about using CSS for the default styles?

@hshoff
Copy link
Member

hshoff commented Apr 9, 2020

Historically, vx has avoided shipping CSS. I would prefer the defaultStyles export because it is lower friction.

As @williaster highlights, extension is easy:

<Tooltip style={{ ...defaultStyles, color: 'magenta' }} ... />

But in terms of API how do folks feel about opting out of the default inline styles?

Option A) Pass empty object

<Tooltip style={{}} ... />

Option B) Prop flag

<Tooltip unstyled />

For option b, the code becomes:

export default function Tooltip({
  className,
  top,
  left,
  unstyled = false,
  style = defaultStyles,
  children,
  ...restProps
}: TooltipProps & JSX.IntrinsicElements['div']) {
  return (
    <div
      className={cx('vx-tooltip-portal', className)}
      style={!unstyled ? style : undefined}
      {...restProps}
    >
      {children}
    </div>
  );
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants