Toolbar implementation in React #115

martinpovolny · 2019-05-15T12:24:46Z

Depends on ~~patternfly/patternfly-react#2041~~ (merged)
~~patternfly/patternfly-react#2984~~ (merged)

TODO:

make the bundle size smaller
~~remove the data- tags~~ (done)

himdel · 2019-05-15T13:32:18Z

src/toolbar/ToolbarButton.jsx

+    name={props.name}
+    title={props.title}
+    className={classNames('btn btn-default', { active: props.selected, disabled: !props.enabled })}
+    data-explorer={props.explorer}


could you get rid of these explicit keys in favor of generic data?

No part of toolbar code (except for miqToolbarOnClick) should be aware that data-function and data-function-data and data-click even exist.. (and all the others).

I guess yes. Just I'd love to get a functional parity with the current implementation first and then improve.

But right, using generic data should be fine as long as we do not handle some data- key in some special way in the current implementation.

I will be removing all the data- tags because the onClick handler now uses props directly. So this will go away.

martinpovolny · 2019-09-19T13:33:18Z

ping @himdel, @rvsia

rvsia · 2019-09-20T08:08:06Z

src/toolbar/ToolbarList.jsx

+
+import CountContext from './ToolbarContext';
+
+// const classNames = require('classnames');


comment here

rvsia · 2019-09-20T08:13:59Z

src/toolbar/Toolbar.jsx

+const toolbarGroupHasContent = group =>
+  group &&
+    group.filter(item => item &&
+      (isButtonOrSelect(item))).length !== 0;


unnecessary parenthesis around isButtonOrSelect

rvsia · 2019-09-20T08:15:54Z

src/toolbar/ToolbarButton.jsx

+
+const classNames = require('classnames');
+
+const ButtonIcon = (props) => {


({ img_url: imgUrl, icon, color, enabled })

rvsia · 2019-09-20T08:16:41Z

src/toolbar/ToolbarButton.jsx

+  enabled: PropTypes.bool,
+};
+
+/* FIXME


What about these comments? Are they still valid?

rvsia · 2019-09-20T08:16:53Z

src/toolbar/ToolbarButton.jsx

+  data: PropTypes.any,
+  selected: PropTypes.bool,
+  enabled: PropTypes.bool,
+  // hidden: PropTypes.bool,


commented prop

rvsia · 2019-09-20T08:23:05Z

src/toolbar/styles.scss

+.kebab .btn-group.open .dropdown-toggle {
+   box-shadow: none;
+}
+


rvsia · 2019-09-20T08:23:52Z

src/toolbar/tests/toolbar.test.js

+
+describe('Toolbar', () => {
+  it('Well it works ;-)', () => {
+    const t = shallow(<Toolbar


I would rename t to something more meaningful

well I like the 't' as toolbar, or the test subject

I think that the short name does not negatively affect readability in this context

rvsia · 2019-09-20T08:24:50Z

src/toolbar/tests/toolbar.test.js

@@ -0,0 +1,97 @@
+import React from 'react';
+import toJson from 'enzyme-to-json';


You don't use it here

rvsia · 2019-09-20T08:31:04Z

src/toolbar/ToolbarButton.jsx

+  id: PropTypes.string,
+  name: PropTypes.string,
+  url: PropTypes.string,
+  url_parms: PropTypes.any,


This triggers Prop type any is forbidden eslint(react/forbid-prop-types)

rvsia · 2019-09-20T08:32:11Z

src/toolbar/tests/toolbar.test.js

+import { ToolbarList } from '../ToolbarList';
+import { ToolbarView } from '../ToolbarView';
+
+const toolbarData = require('../data/toolbar-big.json');


you can use import here

rvsia · 2019-09-20T08:36:07Z

Looks solid and works well in the storybook! 👍 I found only small nitpicks.

rvsia · 2019-09-20T08:48:43Z

Actually missed few UX issues:

Disabled buttons are white, when you make them active (click and hold it)

You can open one item by clicking and you can open the other items by hovering at the same time

Buttons will expand the page, when they don't have space

rvsia · 2019-09-20T09:07:39Z

src/toolbar/ToolbarKebab.jsx

@@ -0,0 +1,78 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+import { DropdownButton, MenuItem } from 'patternfly-react';


Put HorizontalNavMenuItem here

@rvsia

Fix according to @rvsia. Requires patternfly/patternfly-react#2984

martinpovolny · 2019-09-23T13:24:58Z

@rvsia: used your code to fix the multi-open issue (2.), made a small PR to PF3 to fix the console error: patternfly/patternfly-react#2984

martinpovolny · 2019-09-23T17:31:34Z

Fixed UX issue (1.)

I think that the 3rd issue is PF issue and I'd prefer to work on that separately if needed (if it's relevant for ManageIQ where the custom buttons buttons should not be rightmost, I belive).

rvsia · 2019-09-24T07:38:09Z

src/toolbar/ToolbarKebab.jsx

+
+  return (
+    <div className="kebab">
+      <DropdownButton id="menu_kebab" title={<span className="fa fa-ellipsis-v" />}>


Please provide here onClick={() => setIsOpenId(undefined)} to reset the openId when user closes/opens the kebab

rvsia · 2019-09-24T07:38:20Z

src/toolbar/ToolbarKebab.jsx

+  }
+
+  if (item.type === 'buttonSelect') {
+    return <ToolbarSubmenu key={item.id} {...item} onClick={onClick} onSelect={console.log} openId={openId} setIsOpenId={setIsOpenId} />;


console.log here

rvsia · 2019-09-24T07:47:14Z

I still see the first issue

Ad issue 3) If it's not relevant now, I am fine with skipping it.

miq-bot · 2019-09-24T10:12:18Z

Checked commits martinpovolny/react-ui-components@cd4de67~...570de8a with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

martinpovolny · 2019-09-24T10:25:31Z

I cannot tell why the issue got back, was pretty sure I got it right yesterday. However I have redone the rule to be more specific and added important! as the offending rule has that :-(

Now it looks good to me.

Thx!

rvsia

🏆

karelhala · 2019-09-24T11:59:56Z

🎉 This PR is included in version 0.11.47 🎉

The release is available on:

Your semantic-release bot 📦🚀

himdel · 2019-09-24T12:15:36Z

@martinpovolny One more issue I'm seeing in the screenshots - mixing buttons with and without icon in the dropdown causes surprising alignment.

I think we should have 2 cases - no icons in the dropdown would render as is,
but if there's at least one icon, we should align labels under labels, even when there is no icon for that particular label

(I think an empty icon div with .fa-fw (fixed width) should help)

martinpovolny · 2019-09-24T13:25:47Z

Let me take a look into that in a follow-up PR.

himdel reviewed May 15, 2019

View reviewed changes

miq-bot added the wip label May 15, 2019

martinpovolny force-pushed the toolbar branch from 5368dd1 to fc737b4 Compare May 17, 2019 15:35

himdel mentioned this pull request May 21, 2019

[WIP] Collapse custom buttons in the toolbar if they don't fit on screen ManageIQ/ui-components#371

Closed

miq-bot added the unmergeable label May 21, 2019

martinpovolny force-pushed the toolbar branch from db78a66 to f594fb0 Compare July 28, 2019 19:19

miq-bot removed the unmergeable label Jul 29, 2019

martinpovolny mentioned this pull request Aug 13, 2019

Toolbar: React rewrite and simplification. ManageIQ/manageiq-ui-classic#5997

Merged

miq-bot added the unmergeable label Aug 31, 2019

martinpovolny mentioned this pull request Sep 4, 2019

fix(HorizontalNavMenuItem): Allow HorizontalNavMenuItem's title to be a node patternfly/patternfly-react#2825

Merged

martinpovolny force-pushed the toolbar branch from f594fb0 to f960e16 Compare September 4, 2019 11:16

martinpovolny force-pushed the toolbar branch from 1efc5f4 to b0a890e Compare September 17, 2019 10:50

martinpovolny changed the title ~~[WIP] Toolbar implementation in React~~ Toolbar implementation in React Sep 17, 2019

miq-bot removed the wip label Sep 17, 2019

martinpovolny added 13 commits September 17, 2019 13:48

Toolbars: initial implementation of toolbars.

cd4de67

Toolbars: use patternfly-react dropdown.

38b65b6

Toolbars: completing the work.

0ab89f1

Toolbar: custom button group squashing.

806de47

Toolbar: a lot of debugging.

1a3556c

Toolbar: add missing keys, ids and prop types.

7e0f3f2

Toolbar: enable/disable based on "count" and "onwhen".

3aef018

Toolbar: move onClick up in the markup to handle clicks outside text.

2ce70a6

Toolbar: add missing Count context.

dfabebb

Toolbar: fix onwhen "+" handling.

c5030f7

Toolbar: remove support for "custom" content.

c497cc5

Toolbar: add some test coverage.

cfdfdc5

Deduplicate react-dom version.

de0d9d0

miq-bot removed the unmergeable label Sep 17, 2019

martinpovolny added 2 commits September 19, 2019 14:53

Toolbar: do not use data-* attributes.

b9cd5ff

Toolbar: add onViewClick for view buttons.

1717a04

martinpovolny force-pushed the toolbar branch from b0a890e to 1717a04 Compare September 19, 2019 13:32

rvsia reviewed Sep 20, 2019

View reviewed changes

Toolbar: fix opening of multiple items in kebab list.

3a67873

Fix according to @rvsia. Requires patternfly/patternfly-react#2984

martinpovolny force-pushed the toolbar branch from fbbf01d to 8d51825 Compare September 23, 2019 17:29

rvsia reviewed Sep 24, 2019

View reviewed changes

martinpovolny force-pushed the toolbar branch 2 times, most recently from dba82b5 to 91e26f0 Compare September 24, 2019 08:58

martinpovolny added 2 commits September 24, 2019 12:11

Toolbar: fix display of disabled list items on click.

d68cce3

Toolbar: review style fixes.

570de8a

martinpovolny force-pushed the toolbar branch from 91e26f0 to 570de8a Compare September 24, 2019 10:11

rvsia approved these changes Sep 24, 2019

View reviewed changes

himdel merged commit 48af602 into ManageIQ:master Sep 24, 2019

himdel self-assigned this Sep 24, 2019

karelhala added the released label Sep 24, 2019

martinpovolny added this to the Sprint 121 Ending Sep 30, 2019 milestone Sep 24, 2019

h-kataria added the enhancement New feature or request label Oct 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toolbar implementation in React #115

Toolbar implementation in React #115

martinpovolny commented May 15, 2019 •

edited

Loading

himdel May 15, 2019

martinpovolny May 15, 2019

martinpovolny Aug 13, 2019

martinpovolny commented Sep 19, 2019

rvsia Sep 20, 2019

rvsia Sep 20, 2019

rvsia Sep 20, 2019

rvsia Sep 20, 2019

rvsia Sep 20, 2019

rvsia Sep 20, 2019

rvsia Sep 20, 2019

martinpovolny Sep 23, 2019

rvsia Sep 20, 2019

rvsia Sep 20, 2019

rvsia Sep 20, 2019

rvsia commented Sep 20, 2019

rvsia commented Sep 20, 2019

rvsia Sep 20, 2019

martinpovolny commented Sep 23, 2019 •

edited

Loading

martinpovolny commented Sep 23, 2019

rvsia Sep 24, 2019

rvsia Sep 24, 2019

rvsia commented Sep 24, 2019 •

edited

Loading

miq-bot commented Sep 24, 2019

martinpovolny commented Sep 24, 2019 •

edited

Loading

rvsia left a comment

karelhala commented Sep 24, 2019

himdel commented Sep 24, 2019 •

edited

Loading

martinpovolny commented Sep 24, 2019


		import CountContext from './ToolbarContext';

		// const classNames = require('classnames');


		const classNames = require('classnames');

		const ButtonIcon = (props) => {

		@@ -0,0 +1,97 @@
		import React from 'react';
		import toJson from 'enzyme-to-json';

Toolbar implementation in React #115

Toolbar implementation in React #115

Conversation

martinpovolny commented May 15, 2019 • edited Loading

TODO:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpovolny commented Sep 19, 2019

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

rvsia commented Sep 20, 2019

rvsia commented Sep 20, 2019

Choose a reason for hiding this comment

martinpovolny commented Sep 23, 2019 • edited Loading

martinpovolny commented Sep 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvsia commented Sep 24, 2019 • edited Loading

miq-bot commented Sep 24, 2019

martinpovolny commented Sep 24, 2019 • edited Loading

rvsia left a comment

Choose a reason for hiding this comment

karelhala commented Sep 24, 2019

himdel commented Sep 24, 2019 • edited Loading

martinpovolny commented Sep 24, 2019

martinpovolny commented May 15, 2019 •

edited

Loading

martinpovolny commented Sep 23, 2019 •

edited

Loading

rvsia commented Sep 24, 2019 •

edited

Loading

martinpovolny commented Sep 24, 2019 •

edited

Loading

himdel commented Sep 24, 2019 •

edited

Loading