-
Notifications
You must be signed in to change notification settings - Fork 55
feat(Label): added image and imagePosition props #55
Conversation
Codecov Report
@@ Coverage Diff @@
## master #55 +/- ##
==========================================
- Coverage 67.82% 67.09% -0.73%
==========================================
Files 101 101
Lines 1386 1407 +21
Branches 265 290 +25
==========================================
+ Hits 940 944 +4
- Misses 443 459 +16
- Partials 3 4 +1
Continue to review full report at Codecov.
|
<Label | ||
circular | ||
icon="close" | ||
iconPosition="end" |
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 the icon
and the image
are added to the start
position by default, which I think makes sense, then perhaps we call these media
or startMedia
and endMedia
instead?
Trying to think about how this aligns to the similar case of the ListItem. In that case, you can add media
and endMedia
. 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.
@@ -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 comment
The 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.
this.state = { | ||
display: 'inline-block', | ||
} | ||
this.state = {} |
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:
constructor(props) {
super(props)
this.state = {}
}
Is identical to:
state = {}
@@ -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" | |||
description="The Label can contain an image" |
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.
Missing period
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Image is part of the Content
that a Label can contain. This example should be moved from Variations
to Content
.
src/components/Label/Label.tsx
Outdated
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
customPropTypes.itemShorthand
. All shorthand props that create a single item use this custom prop type. It allows string, number, object, element, and is disallowed with children.
src/components/Label/Label.tsx
Outdated
{ | ||
className: classes.image, | ||
...(typeof image === 'string' ? { src: image } : { ...image }), | ||
}, |
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 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 comment
The 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 classes.image
with their className
. This way, if they pass a props.className
, it doesn't override the styling classes:
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' />}
/>
src/components/Label/labelRules.ts
Outdated
@@ -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 comment
The 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)
})
src/components/Label/labelRules.ts
Outdated
...((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 comment
The reason will be displayed to describe this comment to others. Learn more.
Use of !important
is a red flag, is this required? If so, is there a way around it?
# Conflicts: # CHANGELOG.md # docs/src/examples/components/Label/Variations/LabelExampleOnIconClick.shorthand.tsx # src/components/Image/Image.tsx # src/components/Label/Label.tsx
-updated and restructured the examples for the Label component
…iddle -added some variables in the Label component
APIPer our chat, we want to support this API after all: <Label
as='a'
content='Mail'
icon='x'
iconPosition='start' // default for icon and image
image={{ src: '//unsplash.it/50', spacing: 'right', avatar: true }}
imagePosition='end'
/> This allows us to create proper shorthand for icons and images. If we use more generic props, like media, then there is no way to know which component we should create (icon or image?). Image typeLabels with images will be styled like this by default: |
# Conflicts: # CHANGELOG.md # src/components/Label/Label.tsx # src/themes/teams/components/Label/labelStyles.ts
CHANGELOG.md
Outdated
- Add Input `clearable` prop @alinais ([#37](https://github.com/stardust-ui/react/pull/37)) | ||
- Add Label `image` and `imagePosition`, removed `onIconClick` prop @mnajdova ([#55](https://github.com/stardust-ui/react/pull/55/)) |
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.
Move to Unreleased
@@ -7,15 +7,13 @@ import { mountWithProvider } from '../../../utils' | |||
describe('Label', () => { | |||
isConformant(Label) |
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.
You should also test implementsShorthandProp
, see #112.
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.
Added implementsShorthandProp test for the image and icon, please check.
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.
Added 2 comments, once these are dealt with, the PR is good to go.
# Conflicts: # CHANGELOG.md # src/components/Image/Image.tsx # src/components/Label/Label.tsx # src/themes/teams/components/Label/labelStyles.ts
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
const button = mountWithProvider( | ||
<Label icon={{ name: 'close', key: 'icon-key' }} onIconClick={onClick} />, | ||
describe('icon', () => { | ||
it('sets tabIndex="0" if the icon has onClick prop', () => { |
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.
👍
# Conflicts: # CHANGELOG.md
Label
Added image and imagePosition properties to the Label component
TODO
API Proposal
image
Shorthand property for creating image inside the Label component.
imagePosition oneOf(['start', 'end'])
Property for changing the positioning of the image. By default it is set to start