Tagging component #3

PanSpagetka · 2018-02-27T12:09:10Z

How Does it look

Interaction

How does it work

Tags are made up of two parts, a category and value. Some categories allow for more than one value to be associated. Categories which can have assigned only one value are marked with * character.
Tags can be applied to a single item or multiple items at one time depending on the use case.
By selecting Category and Value from select box, you can assign tags to items. You can also remove assigned values by clicking on cross in Tag View.

How to use it

Create Redux store with taggingAppor Add it to your store.
Dispatch action for loading initial state store.dispatch({ initialState: defaultState, type: "UI-COMPONENTS_TAGGING_LOAD_STATE" });
Render Component, you can use either TaggingConnected or TaggingWithButtonsConnected, those are the components which work with Redux and contains logic(they are working).

  ReactDOM.render(
    <Provider store={store}>
      <TaggingWithButtonsConnected />
    </Provider>,
    document.getElementById('demo-app'),
  );

initialState must be structured like this:

tags : [
  { description: 'Number', id: 2, singleValue: true,
    values: [{ description: '1', id: 21 }, { description: '2', id: 22 }] },
  { description: 'Animal', id: 3,
    values: [
      { description: 'Duck', id: 31 }, { description: 'Cat', id: 32 }, { description: 'Dog', id: 33 }
    ] 
  },
], 
assignedTags:  [
  { 
    tagCategory: { description: 'Animal', id: 3 },
    tagValues: [{ description: 'Cat', id: 32 }] 
  }
],
affectedItems : [123456, 654321]

Name	Type	Default	Mandatory	Description
tags	Array	[]	Yes	Categories and its Values which can be assigned to items
description	String	None	Yes	Text description of each Category, this text be showed in Select.
id	Int(but it should work with others)	None	Yes	Unique id used to distinguish Categories between themself.
singleValue	Bool	false	No	Only single value for this category can be assigned at one time. These are marked with `*` in UI.
values	Array	None	Yes	Values which can be assigned to this particular Category. Contains string `description` and Int `id`
assignedTags	Array	[]	No	Categories and it's Values, which are already assigned to `affectedItems`
affectedItems	Array	[]	No	Array of ids identifying items, which tags are edited

TaggingConnected

ohadlevy · 2018-02-27T12:22:31Z

Any reason why this shouldn't be part of patternfly react? I think the pattern is generic enough?

ohadlevy · 2018-02-27T12:33:01Z

/cc @serenamarie125

ohadlevy

looks like a great start :), thank you.

I've added a few inline comments from a very quick code scanning.

a few general comments

I think we should use patternfly-react as much as we can, this includes components that already available, and move the view/react code layers to that repository.
I would suggest you look into how v2v plugin and foreman currently handle redux, I think it would make a lot of sense to be aligned from the beginning vs create technical debt later on.

on a side note, i think it makes sense to send pr's to patternfly-react, as you would get a lot of value out of the review comments while learning react.

ohadlevy · 2018-02-27T12:34:08Z

.eslintrc

@@ -12,6 +12,9 @@
      "ignoreUrls": true,
      "ignoreComments": false
    }],
+    "react/require-default-props": [0],


while i dont have an opinion, maybe linting rule changes should happen in separate pr's so their impact could be discussed?

ohadlevy · 2018-02-27T12:35:31Z

src/tagging/components/tag.jsx

@@ -0,0 +1,29 @@
+import * as React from 'react';
+import PropTypes from 'prop-types';
+ import style from './style';


nitpick: extra whitespace.

ohadlevy · 2018-02-27T12:36:12Z

src/tagging/components/tag.jsx

+import PropTypes from 'prop-types';
+ import style from './style';
+
+class Tag extends React.Component {


imho you dont need a class here, you can simply use a const function instead?

ohadlevy · 2018-02-27T12:36:38Z

src/tagging/components/tag.jsx

+        <span className="label label-info">
+          {this.props.tagCategory}: {this.props.tagValue}
+          <a href="#" onClick={this.handleClick}>
+            <span className="pficon pficon-close" aria-hidden="true" />


please use the icon component from patternfly-react

I will use patternfly/patternfly-react#223 as soon as it's merged

ohadlevy · 2018-02-27T12:36:58Z

src/tagging/components/tag.jsx

+          {this.props.tagCategory}: {this.props.tagValue}
+          <a href="#" onClick={this.handleClick}>
+            <span className="pficon pficon-close" aria-hidden="true" />
+            <span className="sr-only">Remove</span>


all strings should be available as props, so later on you can i18n them.

Done, strings are available as props with default values.

ohadlevy · 2018-02-27T12:37:22Z

src/tagging/components/tagModifier.jsx

+  const tagCategories = Object.keys(props.tags) || [];
+  return (
+    <div className="col-lg-6">
+      <div className="row">


please use col and row components from patternfly-react

ohadlevy · 2018-02-27T12:38:37Z

src/tagging/components/tagModifier.jsx

+import TagSelector from './tagSelector';
+import ValueSelector from './valueSelector';
+
+const TagModifier = (props) => {


please extact the strings, so you can do

const TagModifier = ({ tags, selectedTagCategory, onTagCategroyChange... })

and then just use them without the props.<>

ohadlevy · 2018-02-27T12:40:02Z

src/tagging/reducers/index.js

+import { combineReducers } from 'redux';
+import { modifySetTags, toggle } from './reducers';
+
+const TAGS = {


these should move into fixtures files.

This shouldn't be part of this component, It was there because of testing purposes. I moved it to stories and demo. Now I pass it as a default state, when creating this component

ohadlevy · 2018-02-27T12:40:47Z

src/tagging/reducers/reducers.js

@@ -0,0 +1,24 @@
+// state = state.setTags
+export const modifySetTags = (state = [], action) => {


imho you should use immutable library to avoid changing the existing state.

PanSpagetka · 2018-02-27T12:49:54Z

@ohadlevy I think this could be part of patternfly react in the future, but not yet. First I would like to test it in ManageIQ and see if there are some changes to be made.

After I test it, I would move it to patternfly, preferably to monorepo(when it's done). Because I think this component have very specific usecase and it is not necessary for everybody who uses patternfly to load it.

PanSpagetka · 2018-02-27T13:01:36Z

I have probably should have marked it as WIP, because there is still some work to be done.

@miq-bot add_label WIP

karelhala · 2018-03-01T14:31:52Z

src/tagging/actions/actions.js

@@ -0,0 +1,5 @@
+// action creators
+export const TOGGLE_TAG_CATEGORY_CHANGE = 'TOGGLE_TAG_CATEGORY_CHANGE';


Please add prefix for these actions so we don't clash with other action types.

export const TOGGLE_TAG_CATEGORY_CHANGE = 'UI-COMPONENTS_TOGGLE_TAG_CATEGORY_CHANGE'; export const TOGGLE_TAG_VALUE_CHANGE = 'UI-COMPONENTS_TOGGLE_TAG_VALUE_CHANGE'; export const DELETE_SET_TAG = 'UI-COMPONENTS_DELETE_SET_TAG'; export const ADD_SET_TAG = 'UI-COMPONENTS_ADD_SET_TAG';

serenamarie125 · 2018-03-07T20:14:19Z

@ohadlevy @PanSpagetka I think someone already implemented something in the managieq-ui-service repo in conjunction with UX from a design perspective. @AllenBW I think there was a tag editor modal submitted - I couldn't find a PR to reference. Might you have it handy?

serenamarie125 · 2018-03-07T20:16:04Z

@ohadlevy re: your PF React suggestion, I agree that this would be a great contribution!

AllenBW · 2018-03-07T20:20:23Z

@serenamarie125 yeah!! that was a while back... ManageIQ/manageiq-ui-service#402

serenamarie125 · 2018-03-07T20:22:06Z

Actually, here's the initial piece of it ... although it looks like it never went through a ux review
@beanh66 this is relatively aligned with the design right?

PanSpagetka · 2018-03-08T14:40:44Z

@serenamarie125 Thanks for showing me this, I didn't know that we have official design. But I still think my PR is needed. Even though my knowledge of angular is very poor, I don't think, that somebody can take that Tag editor from ServiceUI and use it in ManageIQ-ui-classic(or anywhere else) easily. It would require some amount of work.

The point of my PR is make reusable component, composed of several simpler components creating some logical structure, using preferable technologies(React, Redux). So you can use this component, if I oversimplify it, with one line of code.

Because logic, data and design are separated, you can change design of the component without affecting it's functionality easily. Or you can simply add another component into it. For example View of Affected Entities, without need for modifying existing code, only adding new.

As I said earlier, I would like to(and will) make PR into Patternfly React, when I have working example in ManageIQ.

If you have some suggestions about design, please let me know.
I hope I have explained why I was(am) making this PR :D

beanh66 · 2018-03-08T17:08:46Z

@serenamarie125 correct, that MIQ implementation is in line with the design we had. Thanks!

serenamarie125 · 2018-03-12T14:28:45Z

@PanSpagetka
Is the screenshot here from your implementation?

Could we get closer to the design shown here https://manageiq.github.io/manageiq-design/UX/common/Tags/design?

Is it possible to have multiple values assigned to the same category? In the design, we are showing a design where we would show that in the "chip".

PanSpagetka · 2018-03-13T13:33:17Z

@serenamarie125 Yes it is. And I will make it look more like in the design. @terezanovotna offered me help, so when I am done with coding I will redesigned it according to the https://manageiq.github.io/manageiq-design/UX/common/Tags/design

Multiple values are not possible now, because there weren't possible in ManageIQ, but I can rewrite it so they will be possible.

karelhala

It needs some refactoring changes so the code is much redable and is less prone to bugs. Not reviewed stories and tests, that will be step 2.

karelhala · 2018-04-05T10:55:09Z

demo/demo-app.jsx


 export default function renderApp() {
-  ReactDOM.render(<cmpA.HelloCmp />, document.getElementById('demo-app'));
+  const store = createStore(taggingApp);


Please use module.hot reload so we can change store on the fly:

if (module.hot) { const store = createStore(taggingApp); module.hot.accept('../src/tagging/reducers', () => { const nextRootReducer = taggingApp; store.replaceReducer(nextRootReducer); }); } store.dispatch(loadState(defaultState)); ReactDOM.render( <TaggingWithButtonsConnected store={store} />, document.getElementById('demo-app'), );

karelhala · 2018-04-05T10:55:53Z

package.json

@@ -91,12 +95,13 @@
  "dependencies": {
    "lodash": "^4.17.4",
    "patternfly-react": "^1.8.0",


Update PF react to newer version "patternfly-react": "^1.16.4",

karelhala · 2018-04-05T10:57:43Z

scripts/webpack.config.js

@@ -31,7 +31,6 @@ module.exports = (env) => {
          enforce: 'pre',
          test: /\.(js|jsx)$/,
          exclude: /node_modules/,
-          loader: 'eslint-loader',


Can you please turn the eslint rule back? You will have few errors, but these are OK and easy to fix.

karelhala · 2018-04-05T10:59:46Z

src/tagging/components/tag.jsx

+const Tag = ({
+  onTagDeleteClick, tagCategory, tagValue,
+}) => (
+  <li key={`${tagValue.id}`}>


You can remove ${..} and use just <li key={tagValue.id}>

karelhala · 2018-04-05T11:00:53Z

src/tagging/components/tag.jsx

+    <Label
+      key={tagValue.id}
+      bsStyle='primary'
+      onRemoveClick={() => { onTagDeleteClick(tagCategory, tagValue); }}


No need to wrap function in body

onRemoveClick={() => onTagDeleteClick(tagCategory, tagValue)}

karelhala · 2018-04-05T11:59:11Z

src/tagging/components/taggingWithButtons.jsx

+  }
+
+  onTagValueChange = (selectedTagValue) => {
+    if (this.props.multiValue) {


Do not create same object twice

const action = { tagCategory: this.props.selectedTagCategory, tagValue: selectedTagValue };

karelhala · 2018-04-05T11:59:44Z

src/tagging/components/taggingWithButtons.jsx

+    }
+  }
+
+  onTagDeleteClick = (tagCategory, tagValue) => {


No need to wrap it up with {}

onTagDeleteClick = (tagCategory, tagValue) => this.props.onTagDeleteClick({ tagCategory, tagValue });

karelhala · 2018-04-05T12:01:14Z

src/tagging/components/taggingWithButtons.jsx

+          selectedTagValue={this.props.selectedTagValue}
+        />
+        <Row className={'pull-right'}>
+          <ButtonGroup>


I would put ButtonGroup in seperate component so we have easier way of combining these two components together.

karelhala · 2018-04-05T12:06:18Z

src/tagging/components/valueSelector.jsx

+
+
+class ValueSelector extends React.Component {
+  handleChange = (selectedOption) => {


Refactor this component to be simpler

handleChange = (selectedOption) => { this.props.onTagValueChange({ description: selectedOption.label, id: selectedOption.value }); }; value = this.props.selectedOption.id; label = this.props.selectedOption.description; tagValues = this.props.tagValues.map(tag => ({ value: tag.id, label: tag.description })); render() { return ( <Select name="form-field-name" value={this.value} label={this.label} onChange={this.handleChange} options={this.tagValues} clearable={false} /> ); }

karelhala · 2018-04-05T12:36:25Z

src/tagging/reducers/reducers.js

+export const modifyassignedTags = (state = [], action) => {
+  switch (action.type) {
+    case actionsConstants.DELETE_ASSIGNED_TAG:
+      return [...state.filter(tag => (tag.tagCategory.id !== action.tag.tagCategory.id)),


Please refactor this to use functions and make it simpler.

PanSpagetka · 2018-04-10T10:40:19Z

@remove_label wip

PanSpagetka · 2018-04-10T10:40:51Z

@miq_bot remove_label wip

PanSpagetka · 2018-04-30T12:45:48Z

@serenamarie125 Thank you for review.

Done
Done
Yes, buttons are optional part of the pattern. You can have exactly same component with or without buttons. First screenshot, with colorful rectangles, is older. I have adjusted button order to be consistent ManageIQ. But the spacing is problematic, I am using button group component which doesn't support spacing. I will look at it later.
Yep, there is.

I have addressed the visual feedback, changed spacing, colors, alignments and updated screenshots/gif.

EDIT:
Use button toolbar for buttons, so they have space between themselves. Update pictures and gif, but for some reason my screenshot tool doesn't capture tooltip. So it is visible only in the gif picture.

miq-bot · 2018-05-02T12:24:16Z

Checked commits PanSpagetka/react-ui-components@fc69c86~...f798b0c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🍰

serenamarie125

@PanSpagetka thanks so much for making all the styling changes. This looks really nice, great job!

miq-bot · 2018-05-03T13:05:38Z

This pull request is not mergeable. Please rebase and repush.

ohadlevy suggested changes Feb 27, 2018

View reviewed changes

PanSpagetka changed the title ~~Tagging component~~ [WIP] Tagging component Feb 27, 2018

karelhala added the wip label Feb 27, 2018

karelhala mentioned this pull request Feb 27, 2018

Move tagging to patternfly-react #5

Closed

PanSpagetka mentioned this pull request Feb 27, 2018

Modify eslint rules #6

Merged

karelhala mentioned this pull request Mar 1, 2018

[POC] refactor(monorepo): Monorepo implementation patternfly/patternfly-react#252

Closed

12 tasks

karelhala reviewed Mar 1, 2018

View reviewed changes

PanSpagetka force-pushed the tagging-component branch 2 times, most recently from b66b4e9 to 546fe8e Compare March 7, 2018 08:43

PanSpagetka force-pushed the tagging-component branch from f056f02 to c168b26 Compare March 19, 2018 13:28

karelhala suggested changes Apr 5, 2018

View reviewed changes

PanSpagetka force-pushed the tagging-component branch 3 times, most recently from 99600ae to d299ee1 Compare April 10, 2018 08:25

PanSpagetka changed the title ~~[WIP] Tagging component~~ Tagging component Apr 10, 2018

miq-bot removed the wip label Apr 10, 2018

PanSpagetka force-pushed the tagging-component branch from d299ee1 to a3cbbf9 Compare April 30, 2018 12:17

PanSpagetka force-pushed the tagging-component branch 2 times, most recently from ca989b8 to f798b0c Compare May 2, 2018 12:19

serenamarie125 approved these changes May 3, 2018

View reviewed changes

miq-bot removed the unmergeable label May 3, 2018

PanSpagetka force-pushed the tagging-component branch from f798b0c to 250d1b2 Compare May 3, 2018 12:59

Add Tagging component

c63373e

miq-bot added the unmergeable label May 3, 2018

PanSpagetka added 11 commits May 3, 2018 15:06

Tests

b87313f

Change css styles

eff1064

Make props passing to tagModifier explicit

6a77b06

Make Tag component const function

94a5c1e

Use Row and Col, change settings for different resolutions

f79ff7e

Extract tags definition, change Demo and Story, use default state

7c3741c

Add support for multi tagging and buttons component

4b7e943

Change layout, multiple values, tests, stories

bfbabef

Cleaning

33df1e0

Review changes

53dc7b8

Address UX review

68c1c06

PanSpagetka force-pushed the tagging-component branch from 250d1b2 to 68c1c06 Compare May 3, 2018 13:07

karelhala merged commit 3d36433 into ManageIQ:master May 3, 2018

miq-bot removed the unmergeable label May 3, 2018

PanSpagetka mentioned this pull request May 3, 2018

Delete string leftovers from rebase #15

Merged

himdel mentioned this pull request May 3, 2018

Add travis defintion, remove example comp #4

Merged

PanSpagetka mentioned this pull request May 15, 2018

[WIP] feat(Tagging) Introduce Tagging React component patternfly/patternfly-react#351

Closed

PanSpagetka mentioned this pull request Aug 3, 2018

feat(CompoundLabel) Add CompoundLabel component patternfly/patternfly-react#464

Merged

ZitaNemeckova mentioned this pull request Jan 22, 2019

Replace Tagging tree with Tagging component ManageIQ/manageiq-ui-classic#5177

Closed

		@@ -0,0 +1,24 @@
		// state = state.setTags
		export const modifySetTags = (state = [], action) => {

		@@ -0,0 +1,5 @@
		// action creators
		export const TOGGLE_TAG_CATEGORY_CHANGE = 'TOGGLE_TAG_CATEGORY_CHANGE';



		class ValueSelector extends React.Component {
		handleChange = (selectedOption) => {

Tagging component #3

Tagging component #3

Conversation

PanSpagetka commented Feb 27, 2018 • edited Loading

How Does it look

Interaction

How does it work

How to use it

TaggingConnected

ohadlevy commented Feb 27, 2018

ohadlevy commented Feb 27, 2018

ohadlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PanSpagetka Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PanSpagetka commented Feb 27, 2018

PanSpagetka commented Feb 27, 2018

Choose a reason for hiding this comment

serenamarie125 commented Mar 7, 2018

serenamarie125 commented Mar 7, 2018

AllenBW commented Mar 7, 2018

serenamarie125 commented Mar 7, 2018

PanSpagetka commented Mar 8, 2018

beanh66 commented Mar 8, 2018

serenamarie125 commented Mar 12, 2018

PanSpagetka commented Mar 13, 2018 • edited Loading

karelhala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PanSpagetka commented Apr 10, 2018

PanSpagetka commented Apr 10, 2018

PanSpagetka commented Apr 30, 2018 • edited Loading

miq-bot commented May 2, 2018

serenamarie125 left a comment

Choose a reason for hiding this comment

miq-bot commented May 3, 2018

PanSpagetka commented Feb 27, 2018 •

edited

Loading

PanSpagetka Feb 28, 2018 •

edited

Loading

PanSpagetka commented Mar 13, 2018 •

edited

Loading

PanSpagetka commented Apr 30, 2018 •

edited

Loading