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

feat: add prop size to VisualPicker component #1527

Conversation

rgah2107
Copy link
Collaborator

@rgah2107 rgah2107 commented May 4, 2020

fix: #1519

Changes proposed in this PR:

@nexxtway/react-rainbow

@commit-lint
Copy link

commit-lint bot commented May 4, 2020

Features

  • add prop size to VisualPicker component (00a6c9a)

Bug Fixes

Contributors

@rgah2107

@@ -13,6 +13,7 @@ export interface VisualPickerProps extends BaseProps {
error?: ReactNode;
children?: ReactNode;
multiple?: boolean;
size?: string;
Copy link
Member

@LeandroTorresSicilia LeandroTorresSicilia May 4, 2020

Choose a reason for hiding this comment

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

be more specific here like:
size?: 'small' | 'medium' | 'large';

props.size === 'small' &&
`
height: 100px;
`};
Copy link
Member

@LeandroTorresSicilia LeandroTorresSicilia May 4, 2020

Choose a reason for hiding this comment

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

here you can use a map like:

const sizeMap = {
    large: '210px',
    medium: '142px',
    small: '100px',
};

and then:
height: ${props => sizeMap[props.size] || 142px}
and thus avoid all conditionals

@TahimiLeonBravo
Copy link
Collaborator

@rgah2107 let's add 2 examples more with the new sizes on the component

const StyledContent = attachThemeAttrs(styled.span)`
height: 142px;
height: ${props => sizeMap[props.size] || '142px'};
Copy link
Contributor

Choose a reason for hiding this comment

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

size is always defined because in defaultProps you put medium

Choose a reason for hiding this comment

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

@yvmunayev
I think we need do a fallback to || '142px' since the user can pass a wrong size value and the component should render nicely

Copy link
Contributor

@yvmunayev yvmunayev May 8, 2020

Choose a reason for hiding this comment

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

@rgah2107 check here, I'm wrong and if necessary put:

height: ${props => sizeMap[props.size] || '142px'};

@TahimiLeonBravo
Copy link
Collaborator

TahimiLeonBravo commented May 6, 2020

@rgah2107 we need to have the same width and the height on the different variants

for example:
if the size="small" the width and height should have 100px
if the size="large" the width and height should have 210px

Screen Shot 2020-05-05 at 9 32 47 PM

Screen Shot 2020-05-05 at 9 32 41 PM

yvmunayev
yvmunayev previously approved these changes May 6, 2020
@yvmunayev yvmunayev dismissed their stale review May 6, 2020 06:21

we need to have the same width and the height on the different variants

height: 142px;
width: 100%;
height: ${props => sizeMap[props.size]};
width: ${props => sizeMap[props.size]};

Choose a reason for hiding this comment

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

here we need to do a fallback to a default size since the user can pass a wrong size where sizeMap[props.size] will return undefined

@TahimiLeonBravo TahimiLeonBravo merged commit 609d3f6 into nexxtway:master May 11, 2020
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.

feat: add prop size to VisualPicker component
4 participants