-
Notifications
You must be signed in to change notification settings - Fork 322
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
feat: Add inverted variant to Tipseen and Steps #1995
Conversation
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.
We need to nest this stylesheet sometime, it would be easier to read and understand
.kindSecondary.colorOnInvertedBackgroundActive, | ||
.kindSecondary.colorOnInvertedBackground:hover, | ||
.kindSecondary.colorOnInvertedBackground:focus { | ||
background: var(--icon-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.
non-semantic color warning
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.
I know, it is the same issue that I talked with you about it yesterday, we don't have semantic colors for the inverted colors :(
@@ -17,7 +19,7 @@ export interface StepsCommandProps extends VibeComponentProps { | |||
stepsCount: number; | |||
isIconHidden?: boolean; | |||
buttonProps?: ButtonProps; | |||
isOnPrimary?: boolean; | |||
color?: StepsColor; |
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.
Isn't it breaking?
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.
In principle, all these components should not be used by themselves, they are the children of the Steps
component where I did not break the api
@@ -14,3 +14,9 @@ export enum StepsDotAriaCurrent { | |||
DATE = "date", | |||
TIME = "time" | |||
} | |||
|
|||
export enum StepsColor { |
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.
Is it possible that we use TS types here instead of enums? Less for the migration
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.
Sure! I forgot we talked about it, I'll change it in all the files I touched
@@ -52,17 +53,17 @@ export const StepsCommand: FC<StepsCommandProps> = ({ | |||
kind={Button.kinds.TERTIARY} | |||
onClick={onClick} | |||
disabled={isDisabled} | |||
color={buttonBaseColor} | |||
// @ts-ignore |
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 ignore here?
color={isOnPrimary ? Button.colors.ON_PRIMARY_COLOR : undefined} | ||
{...finishButtonProps} | ||
> | ||
// @ts-ignore |
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 ignore?
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.
because color
is type StepsColor
and and ButtonColor
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.
Do they have the same values? I wonder if we can do something about it
color={isOnPrimary ? Text.colors.ON_PRIMARY : Text.colors.PRIMARY} | ||
className={cx(styles.numbers)} | ||
>{`${activeStepIndex + 1} \\ ${stepsCount}`}</Text> | ||
// @ts-ignore |
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.
Same
packages/core/src/components/Tipseen/TipseenBasicContent.module.scss
Outdated
Show resolved
Hide resolved
@@ -1,2 +1,2 @@ | |||
export { default as video } from "./video.mp4"; | |||
export { default as picture } from "./picture.png"; | |||
export { default as picture } from "./picture.svg"; |
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.
Nitpick: background.svg or something else haha
### Default | ||
### Colors | ||
|
||
Tipseens come in 2 colors: inverted and primary. \ |
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 slashes? Intended?
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.
Yes, to break a line
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.
Oh didn't know that. Double enter also works no?
> | ||
<div className="monday-storybook-tipseen_column-item monday-storybook-tipseen_hovered"> | ||
<Icon icon={Board} iconSize="24" /> | ||
<span>Sales Pipeline</span> |
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 wondering, is the span needed?
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.
Not sure, I copied the example that already existed and deleted things from it, I'll see if I can remove that too
packages/core/src/components/Tipseen/__stories__/tipseen.stories.helpers.js
Show resolved
Hide resolved
…e.scss Co-authored-by: Tal Koren <talko@monday.com>
https://monday.monday.com/boards/3532714909/pulses/6069570213