-
Notifications
You must be signed in to change notification settings - Fork 405
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/dbeaver/vscode#86/storybook integration #3251
base: devel
Are you sure you want to change the base?
Conversation
aeaf44d
to
60369a4
Compare
… values as default
…tead of many stories
bbfb88d
to
574f786
Compare
"@ariakit/react": "^0.4.15", | ||
"@ladle/react": "^5.0.1", | ||
"@tailwindcss/vite": "^4.0.3", | ||
"@wroud/vite-plugin-asset-resolver": "^0.1.5", |
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.
please keep only major version like ^5
also check for dependencies that it's added correctly to devDependencies
and dependencies
} | ||
|
||
export function Button({ className, variant = 'primary', size = 'medium', loading, children, onClick, ...props }: UiKitButtonProps) { | ||
const classToApply = `dbv-ui-button dbv-ui-button--${variant} dbv-ui-button--${size}` + (className ? ` ${className}` : ''); |
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.
maybe it's better to use a prefix like dbv-kit
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.
kit has less meaning for me than ui, plus ui is one symbol shorter means less html and css. So why should we change it? Can we vote in the team maybe?
} | ||
|
||
export interface ButtonIconProps extends React.HTMLAttributes<HTMLSpanElement> { | ||
position?: 'left' | 'right'; |
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.
this is composition element, doesn't position controlled by inserting component in the right position?:
<Button>
some text
<Button.Icon />
</Button>
<Button>
<Button.Icon />
some text
</Button>
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, you are basically not controlling the position with this props, but styles (subtract padding to balance the button). You may define the prop or keep it empty. It's not ideal, let's discuss
* you may not use this file except in compliance with the License. | ||
*/ | ||
|
||
@layer components { |
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.
file name is incorrect (must be Button)
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.
git didn't want to change it) Fixed.
.dbv-ui-button--small { | ||
padding-inline: calc(var(--btn-padding-inline) * 0.5); | ||
padding-block: calc(var(--btn-padding-block) * 0.5); | ||
font-size: calc(var(--btn-font-size) * 0.875); | ||
height: var(--control-height-small); | ||
} | ||
|
||
.dbv-ui-button--medium { | ||
height: var(--control-height-medium); | ||
font-size: var(--btn-font-size); | ||
} | ||
|
||
.dbv-ui-button--large { | ||
padding-inline: calc(var(--btn-padding-inline) * 1.25); | ||
padding-block: calc(var(--btn-padding-block) * 1.25); | ||
font-size: calc(var(--btn-font-size) * 1.125); | ||
height: var(--control-height-large); | ||
} | ||
|
||
.dbv-ui-button--xlarge { | ||
padding-inline: calc(var(--btn-padding-inline) * 1.5); | ||
padding-block: calc(var(--btn-padding-block) * 1.5); | ||
font-size: calc(var(--btn-font-size) * 1.25); | ||
height: var(--control-height-xlarge); | ||
} |
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 can't we adjust css variable instead, like:
.dbv-ui-button--large {
--btn-padding-inline: calc(var(--btn-padding-inline) * 1.25);
--btn-padding-block: calc(var(--btn-padding-block) * 1.25);
--btn-font-size: calc(var(--btn-font-size) * 1.125);
--btn-height: var(--control-height-large);
}
(this code may not work as expected but shows an idea)
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.
It also may be good to use a global multiplier (so you need to change only one variable)
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.
sorry didn't understand
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.
understood, working on that
import { Button as AriaButton, type ButtonProps } from '@ariakit/react'; | ||
import './Button.css'; | ||
|
||
export interface UiKitButtonProps extends Omit<ButtonProps, 'clickOnEnter' | 'clickOnSpace'> { |
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 ButtonProps
height: 8px; | ||
width: 8px; | ||
height: 16px; | ||
width: 16px; | ||
align-self: flex-start; |
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.
strange changes
height: 24px; | ||
height: 32px; |
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.
strange changes
padding-left: 8px; | ||
justify-content: center; | ||
align-items: center; | ||
|
||
& .staticImage, | ||
& .iconOrImage, | ||
& img { | ||
max-width: 16px; |
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.
strange changes
No description provided.