feat(Label): added image and imagePosition props by mnajdova · Pull Request #55 · microsoft/fluent-ui-react

mnajdova · 2018-08-07T15:04:37Z

Label

Added image and imagePosition properties to the Label component

TODO

API Proposal

image

Shorthand property for creating image inside the Label component.

  <Label content="John Doe" image="public/images/avatar/small/matt.jpg" />

<span class="ui-label ">
  <div class="ui-layout ">
    <div class="ui-layout__start ">
      <img role="presentation" src="public/images/avatar/small/matt.jpg" class="ui-image q r l d"/>
    </div>
    <span class="ui-layout__gap"></span>
    <div class="ui-layout__main">John Doe</div>
  </div>
</span>

imagePosition oneOf(['start', 'end'])

Property for changing the positioning of the image. By default it is set to start

<Label content="John Doe" image="public/images/avatar/small/matt.jpg" imagePosition="end" />

<span class="ui-label ">
  <div class="ui-layout">
    <div class="ui-layout__main">John Doe</div>
    <span class="ui-layout__gap"></span>
    <div class="ui-layout__end ">
      <img role="presentation" src="public/images/avatar/small/matt.jpg" class="ui-image q r l d" />
    </div>
  </div>
</span>

codecov · 2018-08-07T15:05:50Z

Codecov Report

Merging #55 into master will decrease coverage by 0.72%.
The diff coverage is 46.93%.

@@            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

Impacted Files	Coverage Δ
src/themes/teams/components/Layout/layoutStyles.ts	`10.81% <ø> (ø)`	⬆️
src/themes/teams/components/Label/labelStyles.ts	`18.75% <0%> (-18.75%)`	⬇️
...rc/themes/teams/components/Label/labelVariables.ts	`18.18% <0%> (-10.39%)`	⬇️
src/components/Image/Image.tsx	`100% <100%> (ø)`	⬆️
src/components/Label/Label.tsx	`82.22% <72.41%> (-17.78%)`	⬇️
src/components/Layout/Layout.tsx	`94% <0%> (+6%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e56b62c...61aaa6d. Read the comment docs.

…/label-image-prop

levithomason · 2018-08-08T00:00:00Z

docs/src/examples/components/Label/Variations/LabelExampleImage.shorthand.tsx

+  <Label
+    circular
+    icon="close"
+    iconPosition="end"


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?

@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.

levithomason · 2018-08-08T00:00:27Z

docs/src/examples/components/Label/Variations/LabelExampleOnIconClick.shorthand.tsx

 import { Label } from '@stardust-ui/react'

-class LabelExampleIconShorthand extends React.Component<{}, { display: string }> {
+class LabelExampleIconShorthand extends React.Component<any, any> {


No typings in examples, please. They should be geared for minimalism and all consumers.

levithomason · 2018-08-08T00:01:07Z

docs/src/examples/components/Label/Variations/LabelExampleOnIconClick.shorthand.tsx

-    this.state = {
-      display: 'inline-block',
-    }
+    this.state = {}


This:

constructor(props) { super(props) this.state = {} }

Is identical to:

state = {}

levithomason · 2018-08-08T00:01:20Z

docs/src/examples/components/Label/Variations/index.tsx

    />
+    <ComponentExample
+      title="Image"
+      description="The Label can contain an image"


Missing period

levithomason · 2018-08-08T00:02:04Z

docs/src/examples/components/Label/Variations/index.tsx

      examplePath="components/Label/Variations/LabelExampleIconAsShorthand"
    />
+    <ComponentExample
+      title="Image"


Image is part of the Content that a Label can contain. This example should be moved from Variations to Content.

levithomason · 2018-08-08T00:02:52Z

src/components/Label/Label.tsx

    iconPosition: PropTypes.oneOf(['start', 'end']),

+    /** Label can have an icon. */
+    image: customPropTypes.some([PropTypes.string, PropTypes.object]),


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.

levithomason · 2018-08-08T00:06:15Z

src/components/Label/Label.tsx

+        {
+          className: classes.image,
+          ...(typeof image === 'string' ? { src: image } : { ...image }),
+        },


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 }, })

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' />} />

levithomason · 2018-08-08T01:22:15Z

src/components/Label/labelRules.ts


+const getLabelHeight = () => {
+  return pxToRem(20)
+}


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) })

levithomason · 2018-08-08T01:23:04Z

src/components/Label/labelRules.ts

  }),
+  image: () => ({
+    height: `${getLabelHeight()} !important`,
+    width: `${getLabelHeight()} !important`,


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

levithomason · 2018-08-09T21:41:34Z

API

Per 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 type

Labels with images will be styled like this by default:

# Conflicts: # CHANGELOG.md # src/components/Label/Label.tsx # src/themes/teams/components/Label/labelStyles.ts

…sition props

…nClick handler

…i/react into feat/label-image-prop

miroslavstastny · 2018-08-24T13:20:28Z

CHANGELOG.md

-
-## Features
 - 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/))


Move to Unreleased

miroslavstastny · 2018-08-24T13:26:59Z

test/specs/components/Label/Label-test.tsx

@@ -7,15 +7,13 @@ import { mountWithProvider } from '../../../utils'
 describe('Label', () => {
  isConformant(Label)


You should also test implementsShorthandProp, see #112.

Added implementsShorthandProp test for the image and icon, please check.

miroslavstastny

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

kuzhelov · 2018-09-05T11:40:16Z

test/specs/components/Label/Label-test.tsx

-      const button = mountWithProvider(
-        <Label icon={{ name: 'close', key: 'icon-key' }} onIconClick={onClick} />,
+  describe('icon', () => {
+    it('sets tabIndex="0" if the icon has onClick prop', () => {


# Conflicts: # CHANGELOG.md

-added image shorthand to the label component

32f1b81

mnajdova requested review from dvdzkwsk, kuzhelov and levithomason as code owners August 7, 2018 15:04

manajdov and others added 5 commits August 7, 2018 17:07

Merge branch 'master' into feat/label-image-prop

5ab51a9

Merge branch 'master' into feat/label-image-prop

14799ca

-update changelog

500810f

Merge remote-tracking branch 'origin/feat/label-image-prop' into feat…

828a13c

…/label-image-prop

-extract label height in a method

e31bdbc

levithomason reviewed Aug 8, 2018

View reviewed changes

manajdov added 2 commits August 9, 2018 12:03

Merge branch 'master' into feat/label-image-prop

0aeab96

# Conflicts: # CHANGELOG.md # docs/src/examples/components/Label/Variations/LabelExampleOnIconClick.shorthand.tsx # src/components/Image/Image.tsx # src/components/Label/Label.tsx

-replaced image, icon, iconPosition with start and end

c1a7117

-updated and restructured the examples for the Label component

mnajdova changed the title ~~feat(Label): added image shorthand prop~~ feat(Label): added start and end props Aug 9, 2018

manajdov added 2 commits August 9, 2018 13:06

-updated changelog

3249b01

-updated layout styles to align the content int he start and end in m…

d1cbd91

…iddle -added some variables in the Label component

manajdov added 5 commits August 13, 2018 10:14

Merge branch 'master' into feat/label-image-prop

c7ed6cc

# Conflicts: # CHANGELOG.md # src/components/Label/Label.tsx # src/themes/teams/components/Label/labelStyles.ts

-refactoring Label to work with icon, iconPosition, image and imagePo…

ba3b5be

…sition props

-fixes

b3001e4

-updated CHANGELOG

12652e9

-added test for handling the tabIndex property set to the icon with o…

4ca94e6

…nClick handler

mnajdova requested a review from miroslavstastny as a code owner August 23, 2018 09:02

manajdov and others added 4 commits August 23, 2018 11:24

-remove margin from the label styles

211f372

Merge branch 'master' into feat/label-image-prop

b8afd16

Merge branch 'master' into feat/label-image-prop

59b145a

Merge branch 'feat/label-image-prop' of https://github.com/stardust-u…

0d0cfe8

…i/react into feat/label-image-prop

mnajdova added 🚀 ready for review and removed needs author feedback Author's opinion is asked 🚧 WIP labels Aug 23, 2018

miroslavstastny reviewed Aug 24, 2018

View reviewed changes

miroslavstastny approved these changes Aug 24, 2018

View reviewed changes

kuzhelov added needs author changes Author needs to implement changes before merge and removed 🚀 ready for review labels Aug 24, 2018

manajdov added 2 commits August 27, 2018 18:07

Merge branch 'master' into feat/label-image-prop

b0ea6c5

-updated with latest master, fixed changelog, added shorthand tests

ad24d4f

mnajdova added 🚀 ready for review and removed needs author changes Author needs to implement changes before merge labels Aug 27, 2018

manajdov added 6 commits August 30, 2018 11:00

Merge branch 'master' into feat/label-image-prop

3b96351

# Conflicts: # CHANGELOG.md # src/components/Image/Image.tsx # src/components/Label/Label.tsx # src/themes/teams/components/Label/labelStyles.ts

-merge conflicts

bc1d0e6

Merge branch 'master' into feat/label-image-prop

0cb6d1b

-fixed changelog

e31fed7

Merge branch 'master' into feat/label-image-prop

dbafa51

# Conflicts: # CHANGELOG.md

Merge branch 'master' into feat/label-image-prop

e0a86a9

# Conflicts: # CHANGELOG.md

kuzhelov reviewed Sep 5, 2018

View reviewed changes

Merge branch 'master' into feat/label-image-prop

61aaa6d

# Conflicts: # CHANGELOG.md

mnajdova merged commit 45498a4 into master Sep 5, 2018

levithomason deleted the feat/label-image-prop branch October 29, 2019 22:30

This was referenced Mar 11, 2020

Adding 7 new icons microsoft/fluentui#12212

Merged

Updated 'raise hand' icons after bug bash comments microsoft/fluentui#12342

Merged

DustyTheBot mentioned this pull request Nov 16, 2021

fix color for dropdown checkable indicator in hc theme microsoft/fluentui#20621

Merged

2 tasks

		@@ -7,15 +7,13 @@ import { mountWithProvider } from '../../../utils'
		describe('Label', () => {
		isConformant(Label)

Conversation

mnajdova commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Label

TODO

API Proposal

image

imagePosition oneOf(['start', 'end'])

Uh oh!

codecov bot commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levithomason Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levithomason commented Aug 9, 2018

API

Image type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miroslavstastny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mnajdova commented Aug 7, 2018 •

edited

Loading

codecov bot commented Aug 7, 2018 •

edited

Loading

levithomason Aug 8, 2018 •

edited

Loading