feat(Label): add icon, iconPosition and onIconClick props by mnajdova · Pull Request #19 · microsoft/fluent-ui-react

mnajdova · 2018-07-27T13:11:00Z

Label

Creating a removable variation of the Label component by introducing onRemove callback and removeIcon property.

TODO

API Proposal

onRemove

The onRemove prop is a function prop, that will be invoked when clicking on the x icon. The x icon is automatically added if this props is defined.

<Label circular onRemove={this.hide} content="Removable label" />

<label class="ui-label ...">Removable label<i class="ui-icon ..."></i></label>

removeIcon

The removeIcon is shorthand prop for defining the remove icon for the label.

<Label circular onRemove={this.hide} removeIcon="minus" content="Removable label" />

<label class="ui-label ...">Removable label<i class="ui-icon ..."></i></label>

Edit

Thanks to @kuzhelov suggestion the following props will be introduced:

icon:

Shorthand for the icon used in the Label component. This prop can provide just the name of the icon, or object containing the props for the icon:

<Label icon="close" content="Some content" />

or

<Label icon={{name: 'close', key: 'some-key'}} content="Some content" />

<label class="ui-label ..."><i class="ui-icon a b " /> Some content</label>

iconPosition: oneOf('before', 'after')

This property will be used to position the icon before of after the content of the Label.

<Label icon="close" iconPosition="after" content="Some content" />

<label class="ui-label ...">Some content <i class="ui-icon a b " /> </label>

onIconClick: func

This property will be used for propagating callback when the icon is clicked. The same can be achieved if the icon prop provides onClick func:

<Label icon="close" iconPosition="after" onIconClick={someFunction} content="Some content" />

or

<Label icon={{name: 'close' onClick: someFunction}} content="Some content" />

codecov · 2018-07-27T13:14:18Z

Codecov Report

Merging #19 into master will increase coverage by 1.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
+ Coverage   82.94%   84.05%   +1.11%     
==========================================
  Files          59       59              
  Lines         850      878      +28     
  Branches      195      207      +12     
==========================================
+ Hits          705      738      +33     
+ Misses        141      136       -5     
  Partials        4        4

Impacted Files	Coverage Δ
src/components/Label/labelRules.ts	`100% <100%> (ø)`	⬆️
src/components/Label/labelVariables.ts	`100% <100%> (ø)`	⬆️
src/components/Label/Label.tsx	`100% <100%> (ø)`	⬆️
src/components/Icon/Icon.tsx	`100% <100%> (ø)`	⬆️
src/components/Icon/iconRules.ts	`76.47% <0%> (+5.88%)`	⬆️
src/components/Icon/fontAwesomeIconRules.ts	`80% <0%> (+60%)`	⬆️

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 17229f1...23be579. Read the comment docs.

-renamed classes for the removable icon examples

-added tests for handling the onRemove call on clicking the remove icon

# Conflicts: # src/components/Icon/Icon.tsx

kuzhelov · 2018-07-31T07:20:25Z

what I am worrying about a bit is by tying onRemove value with visual representation we are, in fact, tying look with some functional aspects of this component. I would rather expect that, at least at the first stages, we would use an explicit way to declare that Label is 'removable'. The problem that I see with that could be the following. Suppose that we are declaring Label in the following way:

<Label ... onRemove={ onRemoveLogic } />

Suppose, for the sake of example, that onRemoveLogic is not defined till the certain point - for instance, because this logic is provided by parent component and dependent on the context where it is rendered. Then what we could end up being rendered is the following:

originally Label is rendered without any icon (as onRemoveLogic is evaluated to falsy value)
when the value for onRemoveLogic is provided, Label is changing its look

What I would expect instead is that Label will have consistent look through all the time.

Another thing that seems to be a one that requires additional thoought is whether we could do a bit better in providing this functionality as something more generic, something that is able to handle wider set of cases. Once again, for the sake of example, I might think about the same 'Label with Icon' concept to represent the following UI control: a label that has 'plus' icon with drop down menu appearing when 'plus' icon is clicked - in that case it won't be about 'removing' anything, while still we will use the same logic behind.

What we might do to embrace these situations as well is to support the following:

<Label ... icon={{ type='minus' onClick={ .. } }} />

While I do understand that this way of achieving the same effect requires more typing, frankly I would rather prefer to introduce generic functionality, at least at the initial stages - with introducing all the necessary shorthands on top of that afterwards.

Please, @mnajdova, let me know about your thoughts on that. Thanks!

mnajdova · 2018-07-31T09:07:35Z

Roman, thanks for the suggestion. Actually I think you are right, I was just following the convention from SUI, but it wouldn't hurt us to have a simple icon prop with onIconClick and iconPosition props for creating more generic usage of it, or using the icon shorthand that you have given us as example. Will change this to reflect your proposed API.

# Conflicts: # CHANGELOG.md

mnajdova · 2018-07-31T16:12:45Z

So here is how the shorthand icon example looks like now:

Apart from this, we can define the properties for icon using different variables as icon, iconPosition and onIconClick, in the following way:

The the children API for this would look like this:

@kuzhelov please let me know if you have some other suggestion.

kuzhelov · 2018-08-01T16:19:57Z

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

+          onClick: this.hide,
+          variables: { color: 'rgba(0, 0, 0, 0.6)' },
+        }}
+        iconPosition="after"


just as a matter of notice - it seems that we were using start and end as a way to declare position (for example, in Layout component). Maybe, just for the sake of consistency, we should use the same naming here as well

kuzhelov · 2018-08-01T16:22:58Z

src/components/Label/Label.tsx

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


a bit confused - could you, please, suggest why client would need to provide a bool value here?

I will delete this, in the beginning I was thinking about having default value for the removable icon to be close, but it is not applicable anymore. Thanks for the catch!

kuzhelov · 2018-08-01T16:23:42Z

src/components/Label/Label.tsx

+    icon: customPropTypes.some([PropTypes.bool, PropTypes.string, PropTypes.object]),
+
+    /** An icon label can format an Icon to appear before or after the text in the label */
+    iconPosition: PropTypes.oneOf(['before', 'after']),


once again, just for the sake of consistency - maybe it would be better to use start and end keywords for that

I will change these, and create separate PR for fixing this in the button and Icon components.

kuzhelov · 2018-08-01T16:28:22Z

src/components/Label/Label.tsx

    return (
      <ElementType {...rest} className={classes.root}>
-        {childrenExist(children) ? children : content}
+        {getContent()}


I would rather suggest to leave the logic of deciding between rendering children (if exist) or content here - in that way it would be much easier to reason about whether this functionality is provided or not. And getContent() function being responsible to properly compose content nodes in case if content is provided - without taking responsibility on deciding between children and content case:

return ( <ElementType {...rest} className={classes.root}> {childrenExist(children) ? children : getContent()} ...

kuzhelov · 2018-08-01T16:54:47Z

src/components/Label/Label.tsx

+        ...(icon &&
+          typeof icon === 'string' && {
+            name: icon,
+            ...(onIconClick && { tabIndex: '0' }),


it seems to be an important thing - could you, please, kindly suggest what the behavior will be in case if this tabIndex line will be omitted? Thanks :)

The aim here is, to make the icon focusable (having tabindex="0") if eather the onIconClick property is provided or if the icon shorthand provides us with the onClick method for the icon.

kuzhelov · 2018-08-01T17:00:49Z

src/components/Label/Label.tsx

+          typeof icon === 'string' && {
+            name: icon,
+            ...(onIconClick && { tabIndex: '0' }),
+            variables: { color: classes.root.color },


would rather suggest to use variables for this. This will make proper logical ties between these two components - specifically, that icon's color is defined by Label's color variable in this context. And, given that client of Label might be interested in customizing Label's color, we might think about exposing it as a variable (currently hardcoded value is used) - with that we will introduce a way to customize this aspect for Label, a way that is consistent with other components. Please, let me know about your thoughts on this, as well as whether it is better to do that by means of additional dedicated PR

I agree on this, it would be much better. Just to be clear that we are on the same page: I will introduce the color for the label as a color variable inside the label variables, and provide it to the icon in the following manner:

variables: { color: Label.variables().color },

If this is the change that we both aim to, I would rather introduce it in this PR.

kuzhelov · 2018-08-02T08:11:56Z

src/components/Label/Label.tsx

+        ...(icon &&
+          typeof icon === 'object' && {
+            ...icon,
+            ...(icon.onClick && { tabIndex: '0' }),


on a client side, would rather expect that xSpacing will be defined for this case as well, in case if no value is explicitly provided by the client. Probably, it would be better to organise conditional statements in the following way, to reduce code duplication:

...(icon && ...(icon.onClick || onIconClick) && { tabIndex: ... }, ...(getIconContent(icon) ? 'none' : iconAtEnd ? 'before' : 'after') ...

I was not sure if we want to add some other props to the Icon if it is provided as an object, that's why I separated the definition between this two, so that it can be more clear what is applied in both cases. If we decided to apply the same styles to both cases (for the case when the icon is string always, and for the case when the icon is object and those specific are not define), then I guess the reorganizing of the conditional statements can be made. I see the point to finish styling the icon component so that it would look reasonable in the label component, but it might be confusing for the user to see the icon styled with props that he/she has not provided. In the end the only two props that are in question are the color and the xSpacing props for the icon. What is your final thought on this @kuzhelov ?

Actually I refactored this part using the handleIconOverrides method, and I am providing the default values for the props we want to handle if those are not defined. Let me know what you think.

kuzhelov · 2018-08-02T08:22:10Z

src/components/Label/Label.tsx

+            },
+          ),
+      ].filter(Boolean)
+      return iconIsAfterContent ? renderedContent : renderedContent.reverse()


this (starting from line 103) is a neat trick :) but just for the sake of maintainability and readability, it may be better to consider simpler and more expressive approach to declare what will be rendered - just do the following:

renderContent() { ... const icon = renderIcon() const iconAtStart = !iconAtEnd return ( { iconAtStart && icon } { content } { iconAtEnd && icon } ...

to my mind, it would require less thought of the reader to understand what is going on here and whether provided logic is correct in terms of what will be rendered.

Please, let me know what do you think and decide by yourself what would be better :)

-done some refactoring in the render method of the Label component

kuzhelov

couple of notes - please, take a look. Thank you 👍

kuzhelov · 2018-08-03T07:36:09Z

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

+import React from 'react'
+import { Label } from '@stardust-ui/react'
+
+class LabelExampleIconShorthandShorthand extends React.Component<{}, { display: string }> {


nit: Shorthand is used twice in the name

kuzhelov · 2018-08-03T07:38:18Z

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

+
+const LabelExampleIcon = () => (
+  <Label>
+    <Icon name="coffee" xSpacing="after" variables={() => ({ color: 'rgba(0, 0, 0, 0.6)' })} />


note to myself - these keywords for xSpacing should be changed as well

We should introduce separate PR for the Icon's xSpacing and the Button's iconPosition to be consistent with the rest of the components.

kuzhelov · 2018-08-03T07:51:44Z

src/components/Label/Label.tsx

+        _.invoke(this.props, 'onIconClick', e, this.props)
+      },
+      ...((onClick || onIconClick) && { tabIndex: '0' }),
+      ...((!variables || !variables.color) && { variables: { color: Label.variables().color } }),


this turns out to be quite dangerous thing, but seems that we have nothing to do with that now. Essentially, the problem is that Label's variables are evaluated with no argument - while variables fuction implies that there could be site variables provided for this evaluation.

Ideally, this function should have the following semantics:

handleIconOverrides(labelVariables) { // already evaluated .... ...(labelVariables.color && { variables: { color: labelVariables.color }}) }

while for now we will use the same expression for evaluating variables - while keeping in mind that we should find a way to provide evaluated variables at the rendering phase (i.e. as an arg to renderComponent function):

For now

renderComponent(...) { // no evaluated variables are provided // TODO should be fixed const labelVariables = Label.variables() const iconElement = Icon.create({ ... { overrideProps: this.handleOverrideProps(labelVariables) } }

and when fixed:

renderComponent({ variables: labelVariables, ...}) { const iconElement = Icon.create({ ... { overrideProps: this.handleOverrideProps(labelVariables) } }

The overrideProps functino is invoked inside the create factory, so I don't see a way to provide the additional labelVariables from here. The only argument it gets are the predefined props. I can however fix the assumption that the variables are object inside the props of the icon shorthand with the following changes inside the handleOverrideProps like this:

handleIconOverrides = (predefinedProps) => { const { onIconClick, iconPosition, content } = this.props const { onClick, variables, xSpacing } = predefinedProps // evaluate the variables inside the props (supporting variables: () => ({color: 'red'}) and variables: {color: 'red'}) const evaluatedVariables = callable(variables)() return { onClick: e => { _.invoke(predefinedProps, 'onClick', e) _.invoke(this.props, 'onIconClick', e, this.props) }, ...((onClick || onIconClick) && { tabIndex: '0' }), ...((!evaluatedVariables || !evaluatedVariables.color) && { variables: { color: Label.variables().color } }), ...(!xSpacing && { xSpacing: !content ? 'none' : iconPosition === 'end' ? 'before' : 'after', }), } }

What do you think @kuzhelov ?

yes, that would be great - this will save us in situations when variables are provided as plain object. Would rather rename evaluatedVariables to labelVariables - it would be much easier to understand the next lines of code then. Other than that - absolutely agree, great suggestion!

Got, it will do that. And yes, we still need a way to provide the variables together with the site variables inside the overrideProps function, we should have this is mind.

Actually, the latest version of the method would look like this:

handleIconOverrides = (predefinedProps) => { const { onIconClick, iconPosition, content, variables: labelPropVariables } = this.props const { onClick, variables, xSpacing } = predefinedProps const iconVariables = callable(variables)() const labelVariables = labelPropVariables ? callable(labelPropVariables)() : callable(Label.variables)() return { onClick: e => { _.invoke(predefinedProps, 'onClick', e) _.invoke(this.props, 'onIconClick', e, this.props) }, ...((onClick || onIconClick) && { tabIndex: '0' }), ...((!iconVariables || !iconVariables.color) && { variables: { color: labelVariables.color } }), ...(!xSpacing && { xSpacing: !content ? 'none' : iconPosition === 'end' ? 'before' : 'after', }), } }

This way, I am handling the variables inside the label component, the default variables in the Label, as well as the variables inside the icon shorthand. This seems to work with every combination of variables provided in both components.

kuzhelov · 2018-08-03T07:59:25Z

src/components/Label/Label.tsx

+        },
+      )
+
+      return [iconAtStart && icon && iconElement, content, iconAtEnd && icon && iconElement]


it would be a bit problematic for React to monitor changes in case if array elements are not provided with keys. I would rather suggest to use 'React.Fragment' for this scenario (where number of elements to render is rather defined)

<React.Fragment> { iconAtStart && icon && iconElement } { content } { iconAtEnd && icon && iconElement } </React.Fragment>

-added onRemove and removeIcon props in the Label component

3651f7e

mnajdova requested review from dvdzkwsk, kuzhelov and levithomason as code owners July 27, 2018 13:11

mnajdova changed the title ~~feat (Label): removable label~~ feat(Label): removable label Jul 27, 2018

manajdov added 10 commits July 27, 2018 15:35

-added the same color for the remove icon as the label text color

8bff779

-renamed classes for the removable icon examples

-added record in the changelog

f989873

-added tabindex 0 to the remove icon

c555b7e

-deleted children API examples for the removable variation of the Label

f274f5f

-remove children API example for the circular prop

0e44631

-replaced tabindex with tabIndex

3e97c5d

-added tests for handling the onRemove call on clicking the remove icon

-more specific selector for the remove icon

eed20e2

Merge branch 'master' into feat/label-removable

05a1961

# Conflicts: # src/components/Icon/Icon.tsx

-added xSpacing before to the removable icon in the label

ed11d4b

-fixed changelog

10d0743

mnajdova mentioned this pull request Jul 30, 2018

docs: add glossary to specifications #4

Closed

manajdov added 3 commits July 31, 2018 16:17

Merge branch 'master' into feat/label-removable

93cd315

# Conflicts: # CHANGELOG.md

-added icon, iconPosition and onIconClick props in the Label component

c119d3d

-changed CHANGELOG

de4fc7a

manajdov added 3 commits July 31, 2018 18:42

-added icon, iconPosition and onIconClick props in the Label component

62f6d54

-reverted test file

5d0c152

-added test for onIconClick

1cd34e0

mnajdova changed the title ~~feat(Label): removable label~~ feat(Label): add icon, iconPosition and onIconClick props Aug 1, 2018

kuzhelov reviewed Aug 2, 2018

View reviewed changes

manajdov added 2 commits August 2, 2018 16:59

-replaced begin and after values for the iconPosition with start and end

08fdae2

-done some refactoring in the render method of the Label component

-refactored icon definition

d169ce7

kuzhelov approved these changes Aug 3, 2018

View reviewed changes

manajdov and others added 2 commits August 3, 2018 11:13

-fixes

ff1386c

Merge branch 'master' into feat/label-removable

23be579

mnajdova merged commit 41f3b03 into master Aug 3, 2018

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

Conversation

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

Uh oh!

Label

TODO

API Proposal

onRemove

removeIcon

Edit

icon:

iconPosition: oneOf('before', 'after')

onIconClick: func

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

kuzhelov commented Jul 31, 2018

Uh oh!

mnajdova commented Jul 31, 2018

Uh oh!

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

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuzhelov left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mnajdova commented Jul 27, 2018 •

edited

Loading

codecov bot commented Jul 27, 2018 •

edited

Loading

mnajdova commented Jul 31, 2018 •

edited

Loading