Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React: Visualize CreateOrDelete button in table header #10257

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
---
extends: '@elastic/kibana'
rules:
no-unused-vars: off
plugins:
['react']
extends:
['@elastic/kibana',
'plugin:react/recommended']
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

5 changes: 4 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -150,13 +150,16 @@
"mkdirp": "0.5.1",
"moment": "2.13.0",
"moment-timezone": "0.5.4",
"ngreact": "0.3.0",
"no-ui-slider": "1.2.0",
"node-fetch": "1.3.2",
"node-uuid": "1.4.7",
"pegjs": "0.9.0",
"postcss-loader": "1.2.1",
"querystring-browser": "1.0.4",
"raw-loader": "0.5.1",
"react": "15.2.0",
"react-tooltip": "3.2.6",
"request": "2.61.0",
"rimraf": "2.4.3",
"rison-node": "1.0.0",
Expand Down Expand Up @@ -190,6 +193,7 @@
"eslint": "3.11.1",
"eslint-plugin-babel": "4.0.0",
"eslint-plugin-mocha": "4.7.0",
"eslint-plugin-react": "6.9.0",
"event-stream": "3.3.2",
"expect.js": "0.3.1",
"faker": "1.1.0",
Expand Down Expand Up @@ -232,7 +236,6 @@
"npm": "3.10.10",
"portscanner": "1.0.0",
"proxyquire": "1.7.10",
"react": "15.2.0",
"react-addons-test-utils": "15.2.0",
"react-dom": "15.2.0",
"react-redux": "4.4.5",
Expand Down
1 change: 1 addition & 0 deletions src/core_plugins/kibana/public/kibana.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import routes from 'ui/routes';
import modules from 'ui/modules';

import kibanaLogoUrl from 'ui/images/kibana.svg';
import 'ui/react_components';
import 'ui/autoload/all';
import 'plugins/kibana/discover/index';
import 'plugins/kibana/visualize/index';
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import React from 'react';

import { CreateButtonLink } from 'ui_framework/components/button/create_button_link';
import { DeleteButton } from 'ui_framework/components/button/delete_button';

export function CreateOrDeleteButton({ showCreate, doDelete }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (showCreate) {
return <CreateButtonLink
href = "#/visualize/new"
tooltip = "Create new visualization"
/>;
} else {
return <DeleteButton
onClick={() => doDelete() }
tooltip="Delete selected visualizations"
/>;
}
}

CreateOrDeleteButton.propTypes = {
showCreate: React.PropTypes.bool,
doDelete: React.PropTypes.func
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

};
Original file line number Diff line number Diff line change
Expand Up @@ -32,26 +32,10 @@

<div class="kuiToolBarSection">
<!-- Bulk delete button -->
<button
class="kuiButton kuiButton--danger"
aria-label="Delete selected objects"
ng-if="listingController.getSelectedItemsCount() > 0"
ng-click="listingController.deleteSelectedItems()"
tooltip="Delete selected visualizations"
>
<span aria-hidden="true" class="kuiButton__icon kuiIcon fa-trash"></span>
</button>

<!-- Create visualization button -->
<a
class="kuiButton kuiButton--primary"
href="#/visualize/new"
aria-label="Create new visualization"
ng-if="listingController.getSelectedItemsCount() === 0"
tooltip="Create new visualization"
>
<span aria-hidden="true" class="kuiButton__icon kuiIcon fa-plus"></span>
</a>
<create-or-delete-button
show-create="createOrDeleteProps.showCreate"
do-delete="createOrDeleteProps.doDelete"
></create-or-delete-button>
</div>

<div class="kuiToolBarSection">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@ import 'ui/pager_control';
import 'ui/pager';
import _ from 'lodash';

import { CreateOrDeleteButton } from './create_or_delete_button';

import uiModules from 'ui/modules';
const app = uiModules.get('app/visualize', ['react']);
app.directive('createOrDeleteButton', function (reactDirective) {
return reactDirective(CreateOrDeleteButton);
});

export function VisualizeListingController($injector, $scope) {
const $filter = $injector.get('$filter');
const confirmModal = $injector.get('confirmModal');
Expand Down Expand Up @@ -175,4 +183,12 @@ export function VisualizeListingController($injector, $scope) {
this.getUrlForItem = function getUrlForItem(item) {
return `#/visualize/edit/${item.id}`;
};

const updateProps = () => {
$scope.createOrDeleteProps = {
showCreate: this.getSelectedItemsCount() === 0,
doDelete: () => this.deleteSelectedItems()
};
};
$scope.$watch(() => this.getSelectedItemsCount(), () => updateProps());
}
23 changes: 23 additions & 0 deletions src/ui/public/react_components.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import 'ngreact';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


import {
CreateButtonLink,
DeleteButton,
CreateIcon,
DeleteIcon
} from 'ui_framework/components';

import uiModules from 'ui/modules';
const app = uiModules.get('app/kibana', ['react']);
app.directive('createButtonLink', function (reactDirective) {
return reactDirective(CreateButtonLink);
});
app.directive('deleteButton', function (reactDirective) {
return reactDirective(DeleteButton);
});
app.directive('createIcon', function (reactDirective) {
return reactDirective(CreateIcon);
});
app.directive('deleteIcon', function (reactDirective) {
return reactDirective(DeleteIcon);
});
1 change: 1 addition & 0 deletions tasks/config/licenses.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ module.exports = function (grunt) {
'Public domain',
'Unlicense',
'WTFPL',
'(GPL-2.0 OR MIT)',
'new BSD, and MIT'
],
overrides: {
Expand Down
19 changes: 19 additions & 0 deletions ui_framework/components/button/create_button_link.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import React from 'react';

import classnames from 'classnames';
import { KuiButtonLink } from './kui_button_link';
import { CreateIcon } from '../icon';

export function CreateButtonLink(props) {
const { className, ...rest } = props;
const classes = classnames('kuiButton--primary', className);
return <KuiButtonLink className={classes} {...rest}>
<CreateIcon />
</KuiButtonLink>;
}

CreateButtonLink.propTypes = {
tooltip: React.PropTypes.string,
className: React.PropTypes.string,
href: React.PropTypes.string
};
16 changes: 16 additions & 0 deletions ui_framework/components/button/delete_button.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import React from 'react';

import { KuiButton } from './kui_button';
import { DeleteIcon } from '../icon';

export function DeleteButton(props) {
return <KuiButton className="kuiButton--danger" {...props}>
<DeleteIcon />
</KuiButton>;
}

DeleteButton.propTypes = {
tooltip: React.PropTypes.string,
className: React.PropTypes.string,
onClick: React.PropTypes.func
};
24 changes: 24 additions & 0 deletions ui_framework/components/button/kui_button.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import React from 'react';
import classnames from 'classnames';

import { KuiTooltip } from '../tooltip/kui_tooltip';

export function KuiButton({ className, onClick, tooltip, children }) {
const classes = classnames('kuiButton', className);
const button = <button
className={ classes }
aria-label={ tooltip }
onClick={ onClick }
>
{ children }
</button>;

return tooltip ? <KuiTooltip text={ tooltip }>{ button } </KuiTooltip> : button;
}

KuiButton.propTypes = {
tooltip: React.PropTypes.string,
className: React.PropTypes.string,
onClick: React.PropTypes.func,
children: React.PropTypes.array
};
24 changes: 24 additions & 0 deletions ui_framework/components/button/kui_button_link.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import React from 'react';
import classnames from 'classnames';

import { KuiTooltip } from '../tooltip/kui_tooltip';

export function KuiButtonLink({ className, href, tooltip, children }) {
const classes = classnames('kuiButton', className);
const buttonLink = <a
className={ classes }
href={ href }
aria-label={ tooltip }
data-tip={ tooltip }
>
{ children }
</a>;

return tooltip ? <KuiTooltip text={ tooltip }> { buttonLink } </KuiTooltip> : buttonLink;
}

KuiButtonLink.propTypes = {
tooltip: React.PropTypes.string,
href: React.PropTypes.string,
className: React.PropTypes.string
};
6 changes: 6 additions & 0 deletions ui_framework/components/icon/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import React from 'react';
import { KuiIcon } from './kui_icon';

export const DeleteIcon = () => <KuiIcon className="fa-trash"/>;
export const CreateIcon = () => <KuiIcon className="fa-plus"/>;

6 changes: 6 additions & 0 deletions ui_framework/components/icon/kui_icon.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import React from 'react';

export function KuiIcon({ className }) {
const classNames = ['kuiButton__icon', 'kuiIcon', className];
return <span aria-hidden="true" className={ classNames.join(' ') }/>;
}
6 changes: 6 additions & 0 deletions ui_framework/components/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export * from './icon';

export { CreateButtonLink } from './button/create_button_link';
export { DeleteButton } from './button/delete_button';

export { KuiTooltip } from './tooltip/kui_tooltip';
24 changes: 24 additions & 0 deletions ui_framework/components/tooltip/kui_tooltip.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import React from 'react';

import ReactTooltip from 'react-tooltip';

/**
* Wrap elements inside KuiTooltip to provide a popup tooltip on hover.
*
* TODO: This has the unfortunate side affect of attaching a ReactTooltip to every element, when
* it's not neccessary to do so. Unfortunately, because of our angular -> react migration, it's
* not easy to have a single reference to ReactTooltip that every element can reference.
*
* @param text
* @param children
* @returns {XML}
* @constructor
*/
export function KuiTooltip({ text, children }) {
return <div>
<div data-tip={ text } data-for={ text }>
{ children }
</div>
<ReactTooltip place="bottom" id={ text }>{ text }</ReactTooltip>
</div>;
}