-
Notifications
You must be signed in to change notification settings - Fork 0
Feature 7 - Input Component [Dev] #78
base: develop
Are you sure you want to change the base?
Conversation
…omponent requirements
…ute, getting dates inside form tag
You remember add the info to Side Bar as: Reviewers, assigners, Labels, Projects, Milestone and Issue. too, add screenshot of the component in the PR. |
packages/components/package.json
Outdated
@@ -58,6 +58,7 @@ | |||
} | |||
}, | |||
"dependencies": { | |||
"@lerna/bootstrap": "^4.0.0", |
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 we need this package?
const useStyleSheet = createStyleSheet( | ||
(theme, { required, type }) => ({ | ||
inputDefault: { | ||
width: 199, |
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, and the height are givin issues with the checkbox type. They make it looks huge.
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 could provide these styles conditionally.
inputDefault: { | ||
width: 199, | ||
height: 40, | ||
borderRadius: 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.
Don't we have this in the theme?
width: 199, | ||
height: 40, | ||
borderRadius: 5, | ||
display: 'flex', |
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 not gettin this one, I removed those styles and the input behave the same. What are you trying to achieve here?
height: 40, | ||
borderRadius: 5, | ||
display: 'flex', | ||
alignContent: 'center', |
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 also applies to the point above.
borderRadius: 5, | ||
display: 'flex', | ||
alignContent: 'center', | ||
alignItems: 'center', |
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.
And this.
inputStyle: { | ||
padding: theme.spacing(2), | ||
}, | ||
}), { key: 'padding' }); |
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.
Whe could use here DocsInput
instead of padding
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.
The mean of the key is to give a classname prefix.
const { required, type, placeholder, disabled, otherProps } = props; | ||
|
||
return <Input | ||
className={classes.inputStyle} |
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.
Good use of the useStyleSheet hook 😁. We could change the classname to input
instead.
packages/docs/package.json
Outdated
@@ -41,8 +41,9 @@ | |||
"start": "next dev" | |||
}, | |||
"dependencies": { | |||
"@platzily-ui/icons": "0.1.0", | |||
"@lerna/bootstrap": "^4.0.0", |
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 do we need this package?
|
||
const InputComponent = () => { | ||
|
||
return <Input type="checkbox" />; |
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 the one I'm talking that the weight and height are giving troubles. Making it looks huge.
yarn.lock
Outdated
@@ -1446,7 +1446,7 @@ | |||
pacote "^11.2.6" | |||
semver "^7.3.4" | |||
|
|||
"@lerna/bootstrap@4.0.0": | |||
"@lerna/bootstrap@4.0.0", "@lerna/bootstrap@^4.0.0": |
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.
👀
const useStyleSheet = createStyleSheet( | ||
(theme, { width, height }) => ({ | ||
inputDefault: { | ||
width: width || 199, |
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.
Instead of doing this why dont we pass a new class when the button is of type checkbox? 🤔
(theme, { width, height }) => ({ | ||
inputDefault: { | ||
width: width || 199, | ||
height: height || 40, |
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.
Also here.
@@ -15,7 +15,7 @@ const useStyleSheet = createStyleSheet( | |||
(theme, { color, elevation }) => ({ | |||
paper: { | |||
backgroundColor: detectColor(theme, color || 'neutral-tertiary'), | |||
padding: theme.spacing(), | |||
padding: theme.spacing(1), |
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.
No needed when the value is one.
Checklist ✅
resolves [#7 ]