React: Visualize CreateOrDelete button in table header #10257

stacey-gammon · 2017-02-09T01:26:18Z

Taking the ReactIcons one step farther up the DOM and introducing buttons with tooltips.

Works on top of the react icons PR.

stacey-gammon · 2017-02-09T01:34:26Z

UPDATE: Migrated both buttons into a component inside Visualize to avoid the blip, as well as the tooltip prop conflict.

ORIGINAL COMMENT:
@cjcenizal This PR has a blip when hiding/showing the trash and plus icons and I believe it's due to this part of the ui framework style:

  .kuiToolBarSection > * + * {
    margin-left: 10px;
    /* 1 */ }

If I remove it, I'm guessing the angular versions using that code will not work correctly. If that's so, then perhaps I should create a TableActionsComponent to include both the plus and the hide icon and swap it out everywhere, then safely remove that css.

cjcenizal

Nice work! Just had a few suggestions.

cjcenizal · 2017-02-09T22:36:56Z

src/core_plugins/kibana/public/visualize/listing/visualize_listing.html

-        >
-          <span aria-hidden="true" class="kuiButton__icon kuiIcon fa-trash"></span>
-        </button>
+        <react-component name="TrashButtonComponent" props="trashButtonProps"></react-component>


The jitter for these icons is due to both of these elements existing in the DOM at the same time. I think we should use ng-if to conditionally hide/reveal them instead... I tried it out and it fixes the problem:

<react-component name="TrashButtonComponent" props="trashButtonProps" ng-if="listingController.getSelectedItemsCount() > 0" ></react-component> <react-component name="PlusButtonComponent" props="plusButtonProps" ng-if="listingController.getSelectedItemsCount() === 0" ></react-component>

How cool, I didn't know you could use ng-if on the ngreact element. I wonder if that will still work with the reactDirective syntax. I will try it out!

I ended up getting rid of the need for ng-if by combining these both into one react component. The main reason being the tooltip prop conflicted with the angular one and both tooltips were showing. As a bonus, less logic was needed inside the controller, since fewer props are passed.

cjcenizal · 2017-02-09T22:38:36Z

ui_framework/components/button/kui_button.js

+    throw new Error('Only href or onClick can be defined, not both');
+  }
+
+  if (props.hide) return null;


I think we should remove this property and logic. Instead, we can hide elements in Angular using ng-if and in React by omitting them from the render function in the parent, e.g.

render() { let button; if (someCondition === true) { button = <Button />; } return ( <div> Possibly a button: {button} </div> ); }

cjcenizal · 2017-02-09T23:39:24Z

ui_framework/components/button/kui_button.js

+  const classes = classnames('kuiButton', props.className);
+
+  if (props.href) {
+    return <KuiButtonLink {...props} />;


Let's make a hard split between Button and ButtonLink. Can we remove this condition here, disallow the href prop in Button, and disallow onClick in ButtonLink? This way, the engineer has to specify exactly which one to use, instead of delegating the decision to the code.

cjcenizal · 2017-02-09T23:42:15Z

ui_framework/components/button/kui_button.js

+      >
+        { props.children }
+      </button>
+    </KuiTooltip>;


What happens in the event that no tooltip is specified? Shouldn't we just return the element without wrapping it in Tooltip?

export function KuiButton(props) { const classes = classnames('kuiButton', props.className); const button = ( <button className={ classes } aria-label={ props.tooltip onClick={ props.onClick } > { props.children } </button> ); if (props.tooltip) { return <KuiTooltip text={ props.tooltip }>{button}</KuiTooltip>; } else { return button; } }

Another benefit of this approach is that it makes it easier to migrate from many Tooltip instances to using a single Tooltip instance in an owner component. For example, if we create a VisualizeListing component, we can add a Tooltip instance in that component's render method, and then set tooltip to false/undefined for all of the components it owns

The migration would work the same with the current implementation since the logic to not render the ReactTooltip is inside KuiTooltip. I did it this way to avoid adding the conditional logic to everything that might have a tooltip. I guess I don't feel very strongly either way, so I can move it out.

cjcenizal · 2017-02-09T23:42:51Z

ui_framework/components/button/kui_button_link.js

+    { children }
+  </a>;
+
+  return <KuiTooltip text={ tooltip }> { buttonLink } </KuiTooltip>;


Same question here regarding returning a wrapped vs unwrapped element.

cjcenizal · 2017-02-09T23:44:43Z

ui_framework/components/button/plus_button.js

+import React from 'react';
+
+import { KuiButton } from './kui_button';
+import { PlusIcon } from '../icon/plus_icon';


Sorry to make this comment late, but I think we should name these semantically, e.g. PlusIcon -> CreateIcon, PlusButton -> CreateButton, TrashButton -> DeleteButton. This is because we may want to change the icons that represent these actions, and it will be easier to change them within the implementation and have the entire UI update, than to change each use case one-by-one.

Makes sense

stacey-gammon · 2017-02-10T18:37:42Z

src/core_plugins/kibana/public/visualize/index.js

@@ -23,6 +23,14 @@ import { VisualizeConstants } from './visualize_constants';
 import savedObjectRegistry from 'ui/saved_objects/saved_object_registry';
 import savedVisusalizationProvider from 'plugins/kibana/visualize/saved_visualizations/saved_visualization_register';

+import { CreateOrDeleteButton } from './listing/create_or_delete_button';
+
+import uiModules from 'ui/modules';


Question: For app specific react components, should we add the connection at the base of the app (here - index.js) or where they are used (visualize_listing.js). I'm not sure which I like better.

If we want to have the import location consistent with how it's done in React+JSX, we should import the components where they are needed.

I agree with @weltenwort.

stacey-gammon · 2017-02-10T18:39:33Z

src/core_plugins/kibana/public/visualize/listing/create_or_delete_button.js

+import { CreateButtonLink } from 'ui_framework/components/button/create_button_link';
+import { DeleteButton } from 'ui_framework/components/button/delete_button';
+
+export function CreateOrDeleteButton({ showCreate, doDelete }) {


Next step would probably be to throw this into ui_framework and pass in createHref and objectType, so other types can re-use it.

I don't love how there isn't symmetry between the create and delete process. e.g. one is an href one is an action.

Can we avoid destructuring props? I've often had to define a function inside of a stateless functional component, and it's easier to differentiate between these kinds of functions and those provided by props when they are referred to as functionName and props.functionName.

I disagree. I find destructuring much cleaner in most cases.

stacey-gammon · 2017-02-10T18:41:21Z

src/ui/public/react_components.js

@@ -0,0 +1,15 @@
+import 'ngreact';


Question: Should we collect all ui framework components in here, or try to keep this file updated with only those that are currently being used?

IMHO there would be no benefit in implementing but not exporting some components. We won't be able to consistently apply the "currently being used" criterion for long anyway - it'll probably degrade to a mish-mash after a while.

stacey-gammon · 2017-02-10T18:47:00Z

@cjcenizal Updated the code to wrap both buttons in a single react component to simplify some of the code, and avoid conflicts with the tooltip prop.

I also added some question/comments in the code about areas I was unsure of.

kimjoar · 2017-02-13T18:15:33Z

src/core_plugins/kibana/public/visualize/listing/create_or_delete_button.js

+export function CreateOrDeleteButton({ showCreate, doDelete }) {
+  if (showCreate) {
+    return <CreateButtonLink href = "#/visualize/new"
+                             tooltip = "Create new visualization" />;


Not a fan of this kind of aligning. What about this?

return ( <CreateButtonLink href = "#/visualize/new" tooltip = "Create new visualization" /> );

(This works nicely with deeper structures, for examples. It also doesn't break on "search and replace" and things like that. Hopefully prettier becomes production-ready really fast, so we no longer have to think about these kinds of things 🎉)

If we put the closing bracket on its own line, we'll be aligned with the HTML styleguide:

return ( <CreateButtonLink href = "#/visualize/new" tooltip = "Create new visualization" /> );

Let's add https://github.com/yannickcr/eslint-plugin-react 🚀 🌔

kimjoar · 2017-02-13T18:18:33Z

ui_framework/components/button/create_button_link.js

+import { CreateIcon } from '../icon/create_icon';
+
+export function CreateButtonLink(props) {
+  return <KuiButtonLink className="kuiButton--primary" {...props}>


If props contains className it will override the existing className on this. This should probably be:

const { className, ...rest } = props const classes = classnames('kuiButton--primary', className) return <KuiButtonLink className={classes} {...rest}>

or something like that.

kimjoar · 2017-02-13T18:19:06Z

ui_framework/components/button/kui_button.js

+export function KuiButton({ className, onClick, tooltip, children}) {
+  const classes = classnames('kuiButton', className);
+  const button = <button
+      className={ classes }


Should have 2 spaces, not 4, when indenting this

(Maybe we should add https://github.com/yannickcr/eslint-plugin-react?)

TODO: need to implement the tooltips

- Combine the two buttons into a single react component inside visualize. Otherwise the tooltip property became an issue because there is also an angular tooltip prop.

stacey-gammon · 2017-02-13T19:42:14Z

.eslintrc

+  ['react']
+extends:
+  ['@elastic/kibana',
+   'plugin:react/recommended']


@kjbekkelund added the react eslint here. Do you suggest the recommended list? or should I add more?

@spalger am I adding these rules the correct way?

👍 Good start. It doesn't include "stylistic errors". Unless too busy, maybe @cjcenizal can sync up which other options might be relevant based on the html styleguide? (see https://github.com/yannickcr/eslint-plugin-react#jsx-specific-rules, e.g. https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-tag-spacing.md)

In general, though, I think we should just start with this and fix that as we go. This is definitely a good start.

cjcenizal · 2017-02-14T17:09:23Z

src/core_plugins/kibana/public/visualize/listing/create_or_delete_button.js

+
+CreateOrDeleteButton.propTypes = {
+  showCreate: React.PropTypes.bool,
+  doDelete: React.PropTypes.func


Can we change this to isCreate to indicate that it's a boolean and not a function, and onDeleteClick to indicate that it's a callback?

uboness · 2017-02-14T22:35:04Z

I don't think a CreateOrDelete button should be a component. we should have a create button and a delete button and show one of them based on some state... also, some after some feedback, we talked about not hiding the create button after all, even when rows are selected.

The UX we define today is not about switching between a delete and a create tools... but to simply show the appropriate tools for the current context (in the table's case, selection mode)... these tools can be different, depending on the context, the data shown and where it's shown.

stacey-gammon · 2017-02-14T22:58:21Z

Makes sense @uboness and I agree it's awkward. As discussed in Slack, originally I had just two buttons that were controlled via an ng-if. This caused a problem with their tooltips - both angular and react were using the tooltip prop. So throwing them together was a bit of an afterthought to avoid that problem. If the components can be nested one level deeper, I can get rid of that awkardness, since all the code will reside inside react.

I've gotten a lot of great feedback from this PR. How about I abandon any more iterations on it at this time, and start working on the next larger component, which will remove the need to combine these two buttons into one?

@kjbekkelund Let me know what you think - maybe we can zoom some time tomorrow about a good candidate component that can make the official leap.

uboness · 2017-02-14T23:03:35Z

I've gotten a lot of great feedback from this PR. How about I abandon any more iterations on it at this time, and start working on the next larger component, which will remove the need to combine these two buttons into one?

+1 we should definitely not loose all this great feedback moving forward.

stacey-gammon mentioned this pull request Feb 9, 2017

Angular to React Migration #10271

Closed

2 tasks

cjcenizal suggested changes Feb 9, 2017

View reviewed changes

stacey-gammon force-pushed the react-buttons branch from 6271bf8 to 4d60684 Compare February 10, 2017 17:14

stacey-gammon commented Feb 10, 2017

View reviewed changes

stacey-gammon force-pushed the react-buttons branch from 39b5a94 to c48a62a Compare February 10, 2017 18:40

stacey-gammon commented Feb 10, 2017

View reviewed changes

stacey-gammon force-pushed the react-buttons branch from c48a62a to 943625a Compare February 10, 2017 18:42

stacey-gammon requested a review from weltenwort February 10, 2017 18:47

stacey-gammon mentioned this pull request Feb 13, 2017

Angular to React Migration: Leaf Components #10312

Closed

stacey-gammon force-pushed the react-buttons branch from 943625a to 9ecf6a0 Compare February 13, 2017 16:43

stacey-gammon changed the title ~~React buttons~~ React: Visualize CreateOrDelete button in table header Feb 13, 2017

stacey-gammon requested review from kimjoar, tylersmalley, epixa, uboness and spalger February 13, 2017 16:44

stacey-gammon mentioned this pull request Feb 13, 2017

Introducing react icons! #10247

Closed

kimjoar reviewed Feb 13, 2017

View reviewed changes

stacey-gammon added 4 commits February 13, 2017 14:40

Introducing react icons!

fc95a3c

Incorporate buttons

700057d

TODO: need to implement the tooltips

address code review comments

f0187af

- Combine the two buttons into a single react component inside visualize. Otherwise the tooltip property became an issue because there is also an angular tooltip prop.

Address code review comments

8c2d47f

stacey-gammon force-pushed the react-buttons branch from 32f304a to 8c2d47f Compare February 13, 2017 19:41

stacey-gammon commented Feb 13, 2017

View reviewed changes

cjcenizal reviewed Feb 14, 2017

View reviewed changes

address code review comments

045a674

stacey-gammon closed this Feb 22, 2017

stacey-gammon deleted the react-buttons branch October 24, 2017 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React: Visualize CreateOrDelete button in table header #10257

React: Visualize CreateOrDelete button in table header #10257

stacey-gammon commented Feb 9, 2017

stacey-gammon commented Feb 9, 2017 •

edited

Loading

cjcenizal left a comment

cjcenizal Feb 9, 2017

stacey-gammon Feb 10, 2017

stacey-gammon Feb 10, 2017 •

edited

Loading

cjcenizal Feb 9, 2017

cjcenizal Feb 9, 2017

cjcenizal Feb 9, 2017

cjcenizal Feb 9, 2017

stacey-gammon Feb 10, 2017

cjcenizal Feb 9, 2017

cjcenizal Feb 9, 2017

stacey-gammon Feb 10, 2017

stacey-gammon Feb 10, 2017 •

edited

Loading

weltenwort Feb 14, 2017

cjcenizal Feb 14, 2017

stacey-gammon Feb 10, 2017

cjcenizal Feb 14, 2017

kimjoar Feb 14, 2017

stacey-gammon Feb 10, 2017

weltenwort Feb 14, 2017

stacey-gammon commented Feb 10, 2017

kimjoar Feb 13, 2017

cjcenizal Feb 13, 2017

kimjoar Feb 13, 2017

kimjoar Feb 13, 2017

kimjoar Feb 13, 2017

kimjoar Feb 13, 2017

stacey-gammon Feb 13, 2017

kimjoar Feb 13, 2017

cjcenizal Feb 14, 2017

uboness commented Feb 14, 2017 •

edited

Loading

stacey-gammon commented Feb 14, 2017

uboness commented Feb 14, 2017

React: Visualize CreateOrDelete button in table header #10257

React: Visualize CreateOrDelete button in table header #10257

Conversation

stacey-gammon commented Feb 9, 2017

stacey-gammon commented Feb 9, 2017 • edited Loading

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon Feb 10, 2017 • 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

stacey-gammon Feb 10, 2017 • 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

stacey-gammon commented Feb 10, 2017

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

uboness commented Feb 14, 2017 • edited Loading

stacey-gammon commented Feb 14, 2017

uboness commented Feb 14, 2017

stacey-gammon commented Feb 9, 2017 •

edited

Loading

stacey-gammon Feb 10, 2017 •

edited

Loading

stacey-gammon Feb 10, 2017 •

edited

Loading

uboness commented Feb 14, 2017 •

edited

Loading