-
Notifications
You must be signed in to change notification settings - Fork 55
feat(Label): added image and imagePosition props #55
Changes from 6 commits
32f1b81
5ab51a9
14799ca
500810f
828a13c
e31bdbc
0aeab96
c1a7117
3249b01
d1cbd91
c7ed6cc
ba3b5be
b3001e4
12652e9
4ca94e6
29185e8
74a2819
58c47ba
211f372
b8afd16
59b145a
0d0cfe8
b0ea6c5
ad24d4f
3b96351
bc1d0e6
0cb6d1b
e31fed7
dbafa51
e0a86a9
61aaa6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import React from 'react' | ||
import { Label } from '@stardust-ui/react' | ||
|
||
const LabelExampleImageShorthand = () => ( | ||
<Label | ||
circular | ||
icon="close" | ||
iconPosition="end" | ||
content="John Doe" | ||
image={{ src: 'public/images/avatar/small/matt.jpg', avatar: true }} | ||
/> | ||
) | ||
|
||
export default LabelExampleImageShorthand |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,10 @@ | ||
import React from 'react' | ||
import { Label } from '@stardust-ui/react' | ||
|
||
class LabelExampleIconShorthand extends React.Component<{}, { display: string }> { | ||
class LabelExampleIconShorthand extends React.Component<any, any> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No typings in examples, please. They should be geared for minimalism and all consumers. |
||
constructor(props) { | ||
super(props) | ||
this.state = { | ||
display: 'inline-block', | ||
} | ||
this.state = {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This: constructor(props) {
super(props)
this.state = {}
} Is identical to: state = {} |
||
} | ||
|
||
public hide = () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,11 @@ const Variations = () => ( | |
description="The Icon component inside the Label can be defined with customizing it's prop" | ||
examplePath="components/Label/Variations/LabelExampleIconAsShorthand" | ||
/> | ||
<ComponentExample | ||
title="Image" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Image is part of the |
||
description="The Label can contain an image" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing period |
||
examplePath="components/Label/Variations/LabelExampleImage" | ||
/> | ||
</ExampleSection> | ||
) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ import * as _ from 'lodash' | |
|
||
import { childrenExist, createShorthandFactory, customPropTypes, UIComponent } from '../../lib' | ||
|
||
import { Icon } from '../..' | ||
import { Icon, Image } from '../..' | ||
import labelRules from './labelRules' | ||
import labelVariables from './labelVariables' | ||
import callable from '../../lib/callable' | ||
|
@@ -41,6 +41,9 @@ class Label extends UIComponent<any, any> { | |
/** An icon label can format an Icon to appear before or after the text in the label */ | ||
iconPosition: PropTypes.oneOf(['start', 'end']), | ||
|
||
/** Label can have an icon. */ | ||
image: customPropTypes.some([PropTypes.string, PropTypes.object]), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
/** | ||
* Function called when the icon is clicked. | ||
* | ||
|
@@ -58,6 +61,7 @@ class Label extends UIComponent<any, any> { | |
'content', | ||
'icon', | ||
'iconPosition', | ||
'image', | ||
'onIconClick', | ||
] | ||
|
||
|
@@ -94,7 +98,7 @@ class Label extends UIComponent<any, any> { | |
} | ||
|
||
renderComponent({ ElementType, classes, rest }) { | ||
const { children, content, icon, iconPosition } = this.props | ||
const { children, content, icon, iconPosition, image } = this.props | ||
const getContent = (): React.ReactNode => { | ||
const iconAtEnd = iconPosition === 'end' | ||
const iconAtStart = !iconAtEnd | ||
|
@@ -110,8 +114,19 @@ class Label extends UIComponent<any, any> { | |
}, | ||
) | ||
|
||
const imageElement = Image.create( | ||
{ | ||
className: classes.image, | ||
...(typeof image === 'string' ? { src: image } : { ...image }), | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not handle other cases of shorthand, such as passing elements. Instead of attempting to figure out which type of shorthand was passed, you can leave this to the factory. To pass additional defaultProps, such as the className in this case, you can use the options object: Image.create(image, {
generateKey: false,
defaultProps: { className: classes.image },
}) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, it may be required to override the user's props and merge the handleImageOverrideProps = props => ({
...props,
className: cx(classes.image, props.className),
})
Image.create(image, {
generateKey: false,
overrideProps: this.handleImageOverrideProps,
}) Now, the component will override any props.className provided by the user by merging the style className into them. Bonus: What if the user doesn't want our classNames?! Answer: They can use a function for shorthand and whatever they want: <Label
image={(Image, props) => <Image {...props} className='only these' />}
/> |
||
{ | ||
generateKey: false, | ||
}, | ||
) | ||
|
||
return ( | ||
<React.Fragment> | ||
{image && imageElement} | ||
{iconAtStart && icon && iconElement} | ||
{content} | ||
{iconAtEnd && icon && iconElement} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,39 @@ | ||
import { pxToRem } from '../../lib' | ||
|
||
const getLabelHeight = () => { | ||
return pxToRem(20) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems this can just be a constant value: const labelHeight = pxToRem(20) Perhaps it should even be a variable? // labelVariables.ts
export default () => ({
height: pxToRem(20)
}) |
||
|
||
export default { | ||
root: ({ props, variables }) => ({ | ||
height: getLabelHeight(), | ||
margin: `${pxToRem(4)} 0 0 ${pxToRem(4)}`, | ||
padding: variables.padding, | ||
fontWeight: '500', | ||
display: 'inline-block', | ||
fontSize: pxToRem(14), | ||
lineHeight: getLabelHeight(), | ||
verticalAlign: 'middle', | ||
backgroundColor: 'rgb(232, 232, 232)', | ||
color: variables.color, | ||
borderRadius: pxToRem(3), | ||
...(props.circular && { | ||
borderRadius: variables.circularRadius, | ||
}), | ||
overflow: 'hidden', | ||
}), | ||
icon: ({ props }) => ({ | ||
position: 'relative', | ||
top: '-0.15em', | ||
top: `-${pxToRem(2)}`, | ||
...((props.onIconClick || | ||
(props.icon && typeof props.icon === 'object' && props.icon.onClick)) && { | ||
cursor: 'pointer', | ||
}), | ||
}), | ||
image: () => ({ | ||
height: `${getLabelHeight()} !important`, | ||
width: `${getLabelHeight()} !important`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use of |
||
verticalAlign: 'top', | ||
position: 'relative', | ||
left: `-${pxToRem(4)}`, | ||
}), | ||
} |
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.
What happens if you add an icon, but not
iconPosition="end"
? If both theicon
and theimage
are added to thestart
position by default, which I think makes sense, then perhaps we call thesemedia
orstartMedia
andendMedia
instead?Trying to think about how this aligns to the similar case of the ListItem. In that case, you can add
media
andendMedia
. The layout of both a ListItem and a Label are idential in this regard. The start and end media pieces collapse their width while the main area expands to fill the space.The flip side of this argument is that perhaps it might be more common and more intuitive to add an image to the "start" side by default but maybe icons are most intuitive on the "end" side by default. Therefore, a position prop is introduced to allow positioning them on the less common/intuitive side when needed.
Feedback?
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.
@levithomason I already created PR for the ItemLayout component, and yes, after working with it I tried to re-implement the Label with the startMedia and endMedia and it it much cleaner. With that we would get rid of the icon, iconPosition and image props and have more abstract way of defining the start and end media in the Label, as well as other components. My proposal is, to wait for the ItemLayout component PR to be merged and immediately after it, I will publish the PR for the Label containing the start and end media, as I already have it ready on a separate branch.