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: removing border from tooltip arrow + adding themes to storybook #530

Merged
merged 4 commits into from
Feb 15, 2022

Conversation

etaylib
Copy link
Contributor

@etaylib etaylib commented Feb 14, 2022

  1. removing the border from the tooltip's arrow as it added some weird border around it.
    from
    image
    to
    image

  2. adding tooltip themes to storybook

  3. exposing white theme

image

@etaylib etaylib changed the title removing border from tooltip arrow tooltip: removing border from tooltip arrow + adding themes to storybook Feb 14, 2022
src/components/Tooltip/__stories__/tooltip.stories.mdx Outdated Show resolved Hide resolved

Tooltip’s arrow can appear from top, bottom, left or right.

<Canvas>
Copy link
Contributor

@laviomri laviomri Feb 15, 2022

Choose a reason for hiding this comment

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

CC @hadasfa WDYT about the following implementation?

<Canvas>
  <Story name="Themes">
    {Object.values(Tooltip.themes).map(theme => (
      <div className="monday-storybook-tooltip_top" key={theme}>
        <Tooltip
          modifiers={modifiers}
          content={capitalize(theme)}
          shouldShowOnMount
          position={Tooltip.positions.BOTTOM}
          animationType="expand"
          theme={theme}
        >
        <div/>
        </Tooltip>
      </div>
    ))}
  </Story>
</Canvas>

It's shorter , and will automatically support new themes. However, the code snippet doesn't look too good:
image

I think it may be worth it, since this example will be used mainly to visually see all the themes, and not as a code reference. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Like 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.

just aligned with the others..will change

@@ -5,7 +5,8 @@ export const TOOLTIP_THEMES = {
Share: "share",
Private: "private",
Surface: "surface",
Primary: "primary"
Primary: "primary",
White: "white"
Copy link
Contributor

Choose a reason for hiding this comment

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

why the new name include the explicit color in its name? we always trying to choose names according to usage and not to explicit color.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's try to find a naming which indicate its usage purpose?

Copy link
Contributor

@hadasfa hadasfa left a comment

Choose a reason for hiding this comment

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

Wrote a few notes :)

@hadasfa hadasfa self-requested a review February 15, 2022 10:41
@hadasfa hadasfa merged commit 511ddca into master Feb 15, 2022
@hadasfa hadasfa deleted the feature/etay/tooltip/remove-border-from-arrow branch February 15, 2022 10:41
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