-
Notifications
You must be signed in to change notification settings - Fork 317
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
tooltip: removing border from tooltip arrow + adding themes to storybook #530
Conversation
|
||
Tooltip’s arrow can appear from top, bottom, left or right. | ||
|
||
<Canvas> |
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.
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:
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?
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.
Like 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.
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" |
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.
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.
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.
let's try to find a naming which indicate its usage purpose?
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.
Wrote a few notes :)
removing the border from the tooltip's arrow as it added some weird border around it.
from
to
adding tooltip themes to storybook
exposing white theme