Introducing react icons! #10247

stacey-gammon · 2017-02-08T17:38:48Z

The very first introduction of embedded react in our angular apps.

Starting very simple with components that have no props passed from angular, or state. I've confirmed it's possible, just decided to start very small.

spalger · 2017-02-08T19:16:14Z

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

+
+import uiModules from 'ui/modules';
+const app = uiModules.get('app/visualize', ['react']);
+app.value('PlusIcon', PlusIcon);


I wonder if we can come up with a naming convention that distinguishes react components from all other other values in the injector... maybe XYZComponent?

I'd be down with that.

cjcenizal

Functionality and UI looks good! This is awesome. Just had a couple suggestions.

cjcenizal · 2017-02-08T20:09:04Z

src/ui/public/react_components.js

+import 'ngreact';
+
+import { PlusIcon } from 'ui_framework/components/icon/plus_icon';
+import { TrashIcon } from 'ui_framework/components/icon/trash_icon';


Can we implement an index.js file in ui_framework/components which imports and re-exports the components?

export { PlusIcon } from './icon/plus_icon'; export { TrashIcon } from './icon/trash_icon';

And then we can import them here, like this:

import { PlusIcon, TrashIcon, } from 'ui_framework/components';

I think this is an improvement because then we can treat the UI Framework like a module, and we'll have the ability to rearrange things internally by renaming folders and moving files without changing the interface. We can also write tests for this interface for immediate notification of when we break it by accident.

cjcenizal · 2017-02-08T20:11:13Z

ui_framework/components/icon/kui_icon.js

+
+export function KuiIcon({ className }) {
+  const classNames = ['kuiButton__icon', 'kuiIcon', className];
+  return <span aria-hidden="true" className={ classNames.join(' ') }/>;


Can we add the classnames dependency and use it here instead of using join? It will become more useful when we have more complex logic for adding class names, e.g. setting a special "clickable" class to labels if they're associated with an input via the for attribute.

import classNames from 'classnames'; const classes = classNames('kuiButton__icon kuiIcon', classname);

it's yet another dependency just for a single function... we can copy the function, put it in our utils and maintain it ourselves.

spalger

LGTM

cjcenizal

LGTM!

weltenwort

So glad to see react in the package.json! 🎉

weltenwort · 2017-02-09T10:15:49Z

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

@@ -39,7 +39,7 @@
          ng-click="listingController.deleteSelectedItems()"
          tooltip="Delete selected visualizations"
        >
-          <span aria-hidden="true" class="kuiButton__icon kuiIcon fa-trash"></span>
+          <react-component name="TrashIconComponent"></react-component>


Was a conscious choice made here about using <react-component> over the reactDirective Service? The latter seems to allow more control over the way bindings are converted to props.

Nope, just unaware of that, very cool!! Switched over.

Nice! Love the change

weltenwort

LGTM, love the use of stateless components there 👍

stacey-gammon · 2017-02-09T16:04:06Z

Jenkins error:

16:01:01.169: Taking screenshot "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/screenshots/failure/failure_1486656061169_discover app_discover app.png"
SUITE ERROR
Error: [mapper_parsing_exception] [include_in_all] is not allowed for indices created on or after version 6.0.0 as [_all] is deprecated. As a replacement, you can use an [copy_to] on mapping fields to create your own catch all field.
  at  :: {"path":"/logstash-2015.09.20","query":{},"body":"{\"settings\":{\"index\":{\"number_of_shards\":1,\"number_of_replicas\":0},\"analysis\":{\"analyzer\":{\"url\":{\"type\":\"standard\",\"tokenizer\":\"uax_url_email\",\"max_token_length\":1000}}}},\"mappings\":{\"_default_\":{\"dynamic_templates\":[{\"string_fields\":{\"mapping\":{\"type\":\"text\",\"fields\":{\"raw\":{\"type\":\"keyword\"}}},\"match_mapping_type\":\"string\",\"match\":\"*\"}}],\"properties\":{\"  <timestamp\":{\"type\":\"date\"},\"id\":{\"type\":\"integer\",\"index\":true,\"include_in_all\":false},\"clientip\":{\"type\":\"ip\"},\"ip\":{\"type\":\"ip\"},\"memory\":{\"type\":\"double\"},\"referer\":{\"type\":\"keyword\"},\"geo\":{\"properties\":{\"srcdest\":{\"type\":\"keyword\"},\"dest\":{\"type\":\"keyword\"},\"src\":{\"type\":\"keyword\"},\"coordinates\":{\"type\":\"geo_point\"}}},\"meta\":{\"properties\":{\"related\":{\"type\":\"text\"},\"char\":{\"type\":\"keyword\"},\"user\":{\"properties\":{\"firstname\":{\"type\":\"text\"},\"lastname\":{\"type\":\"integer\",\"index\":true}}}}}}}}}","statusCode":400,"response":"{\"error\":{\"root_cause\":[{\"type\":\"mapper_parsing_exception\",\"reason\":\"[include_in_all] is not allowed for indices created on or after version 6.0.0 as [_all] is deprecated. As a replacement, you can use an [copy_to] on mapping fields to create your own catch all field.\"}],\"type\":\"mapper_parsing_exception\",\"reason\":\"Failed to parse mapping [_default_]: [include_in_all] is not allowed for indices created on or after version 6.0.0 as [_all] is deprecated. As a replacement, you can use an [copy_to] on mapping fields to create your own catch all field.\",\"caused_by\":{\"type\":\"mapper_parsing_exception\",\"reason\":\"[include_in_all] is not allowed for indices created on or after version 6.0.0 as [_all] is deprecated. As a replacement, you can use an [copy_to] on mapping fields to create your own catch all field.\"}},\"status\":400}"}>
    at respond (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/node_modules/elasticsearch/src/lib/transport.js:289:15)
    at checkRespForFailure (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/node_modules/elasticsearch/src/lib/transport.js:248:7)
    at HttpConnector.<anonymous> (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/node_modules/elasticsearch/src/lib/connectors/http.js:164:7)
    at IncomingMessage.wrapper (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/node_modules/elasticsearch/node_modules/lodash/lodash.js:4968:19)
    at emitNone (events.js:91:20)
    at IncomingMessage.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:974:12)
    at _combinedTickCallback (internal/process/next_tick.js:74:11)
    at process._tickCallback (internal/process/next_tick.js:98:9)
>> 19/19 tests failed

looks unrelated. Trying again.

jenkins test this

spalger · 2017-02-09T19:38:27Z

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

@@ -39,7 +39,7 @@
          ng-click="listingController.deleteSelectedItems()"
          tooltip="Delete selected visualizations"
        >
-          <span aria-hidden="true" class="kuiButton__icon kuiIcon fa-trash"></span>
+          <trash-icon-component></trash-icon-component>


So... this is pretty different than it was so I think we should rethink this naming convention :) <kui-trash-icon>?

I like that idea, but keep in mind not all react components will be coming from the ui framework. I think that's a great idea for ui framework components, but what about other components? E.g. <visualize-landing-table></visualize-landing-table>

I agree that the component names should not contain "Component". IHMO <visualize-landing-table> sounds ok as does <kui-trash-icon>.

Personally, I don't think there is any reason to indicate that these angular directives are backed by react components. When the react components were sitting in the angular $injector, it was a different story, but now I think it makes more sense to hide that detail and just treat them like every other directive

thomasneirynck · 2017-02-09T22:25:07Z

rebasing with master should get CI to pass again (cf. 0d9e51f)

stacey-gammon · 2017-02-13T16:48:13Z

@kjbekkelund This was my first PR (not to be checked in) that tackled react components in their simplest form - that didn't require props or state at all. Feel free to comment on this PR, or just go straight to the next PR: #10257 which consumes this one and adds on to it.

kimjoar · 2017-02-13T18:11:41Z

ui_framework/components/icon/delete_icon.js

+import { KuiIcon } from './kui_icon';
+
+export function DeleteIcon() {
+  return <KuiIcon className="fa-trash"/>;


Nitpick: As these become a whole lot of tiny files, I think I'd personally go for starting with adding them to ui_framework/components/icon/index.js like this:

import React from 'react'; import { KuiIcon } from './kui_icon'; export const DeleteIcon = () => <KuiIcon className="fa-trash"/> export const CreateIcon = () => <KuiIcon className="fa-plus"/>

etc, and in ui_framework/components/index.js:

export * from './icon';

then later on extract even further if needed. That would also make it easy to extract one icon into it's own file and do something like this:

export { UserIcon } from './user_icon'

instead of defining it in the same file.

As these become a whole lot of tiny files, I think I'd personally go for starting with adding them to ui_framework/components/icon/index.js like this:

Personally I don't mind separate files (as you can guess... I like consistency).

Also, I wonder if we should use CamelCasing for the component file names (will clearly indicate that a file contains a component). In this example, DeleteIcon.js, CreateIcon.js, KuiIcon.js, etc...

then later on extract even further if needed. That would also make it easy to extract one icon into it's own file and do something like this:

How about just always using named imports?

What does the exports have to do with named imports? You'd still import them in the same way as before, this would just be an "implementation detail" within the ui_framework/components/icon folder.

What does the exports have to do with named imports? You'd still import them in the same way as before, this would just be an "implementation detail" within the ui_framework/components/icon folder.

Oops, sorry... I thought you showed imports.

Also, I wonder if we should use CamelCasing for the component file names (will clearly indicate that a file contains a component). In this example, DeleteIcon.js, CreateIcon.js, KuiIcon.js, etc...

I much prefer CamelCasing file names, but I think there may have been a reason we use underscore? I was chatting with... I think @cjcenizal about this. Case sensitivity on different OS's or something? Anyway I am totally in favor of that, if there are no relevant objections. Just want to pull the right people in who have more background then me. @spalger?

++ @kjbekkelund on the exports suggestion.

aha.. .found it :)

in any case… for me everything is open a part the move to React…. if we feel that it’s more appropriate to use CamelCasing names for react components… it’s something we can open. Also, if we do want to follow airbnb style guilde as close as possible, that’s what they recommend as well (https://github.com/airbnb/javascript/tree/master/react#naming) (note though, that we should avoid using .jsx extensions, as React is moving away from those too).

We made a decision many months ago to switch all file names to underscores regardless of purpose, and I think we should stick with that decision in the context of this PR. For one, it wasn't arbitrary: file name case sensitive used to bite us (well, developers on windows) with at least some regularity. I seem to recall that same issue happening recently on one of our dependencies that we don't force this constraint on... maybe the ui framework?

Changing file name conventions is also disruptive to anyone doing development on Kibana since JS imports are explicit. There's no autoloader or anything like that for us to use to help mask the nature of the change to consuming code. The shift to the current file names across the codebase was a pretty unpopular change initially as it broke a ton of stuff in the wild.

Given the difficulties around undoing file name conventions, I'm concerned about the idea that our dependencies dictate our file naming conventions.

@epixa these are all valid points

As for platform issues with camel casing. I've been developing Java for many years not, within small and large teams, on many platforms... never encounter an issue when it comes to camel casing file names. The only problems I encountered relate to Git, when needed to change the casing of the file - Git doesn't handle it well, but there are ways around that. The only reason that can come to mind why one would say that case insensitive platforms are a problem is the a case where you have two files with the same name yet different casing... but that is just plain wrong to even be applicable as a reason.

I also find it interesting that while many follow the CamelCasing convention for file names, we find it unworkable due to platform compatibility.

Given the difficulties around undoing file name conventions, I'm concerned about the idea that our dependencies dictate our file naming conventions.

nothing dictates anything here. If there are common conventions its a good practice to follow them. If we decide to move away from common conventions, that's fine too... but ideally we shouldn't IMO...

that said, I get your point around the effort for it... but big efforts should not hold us back from doing the right thing (if we decide it's the right thing that is)

Let's talk about this in person. There's going to be a ton of back and forth here, and we have that opportunity coming up in March. For now, let's not block this react progress on it. We can always do a small code shift to fix these new changes in March if that's what we decide.

The only reason that can come to mind why one would say that case insensitive platforms are a problem is the a case where you have two files with the same name yet different casing... but that is just plain wrong to even be applicable as a reason.

Just to note, I've wasted hours debugging this exact issue before. It does happen, so it is a valid argument.

stacey-gammon force-pushed the react-icons branch 2 times, most recently from 78c23f8 to 43c4c60 Compare February 8, 2017 18:21

spalger reviewed Feb 8, 2017

View reviewed changes

stacey-gammon force-pushed the react-icons branch from 43c4c60 to eabd33a Compare February 8, 2017 19:35

stacey-gammon requested review from cjcenizal and weltenwort February 8, 2017 19:35

cjcenizal suggested changes Feb 8, 2017

View reviewed changes

spalger approved these changes Feb 8, 2017

View reviewed changes

cjcenizal approved these changes Feb 9, 2017

View reviewed changes

weltenwort reviewed Feb 9, 2017

View reviewed changes

weltenwort approved these changes Feb 9, 2017

View reviewed changes

stacey-gammon mentioned this pull request Feb 9, 2017

Angular to React Migration #10271

Closed

2 tasks

spalger reviewed Feb 9, 2017

View reviewed changes

stacey-gammon force-pushed the react-icons branch from a6eab31 to 31c3dc9 Compare February 10, 2017 17:13

stacey-gammon mentioned this pull request Feb 13, 2017

Angular to React Migration: Leaf Components #10312

Closed

kimjoar reviewed Feb 13, 2017

View reviewed changes

kimjoar approved these changes Feb 13, 2017

View reviewed changes

stacey-gammon force-pushed the react-icons branch 2 times, most recently from 973c52f to 3ff05bd Compare February 14, 2017 19:05

stacey-gammon added 6 commits February 14, 2017 14:06

Introducing react icons!

7dc9308

address code review comments

0b811fc

Use reactDirective instead of react-component

01a3cdf

rename icons

9326442

add missing files

6280d72

Move icon functions to index.js file

15e1a77

stacey-gammon force-pushed the react-icons branch from 3ff05bd to 15e1a77 Compare February 14, 2017 19:06

stacey-gammon closed this Feb 22, 2017

stacey-gammon mentioned this pull request May 2, 2017

UiFramework: Modals in react #11500

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing react icons! #10247

Introducing react icons! #10247

stacey-gammon commented Feb 8, 2017 •

edited

Loading

spalger Feb 8, 2017

stacey-gammon Feb 8, 2017

cjcenizal left a comment

cjcenizal Feb 8, 2017

cjcenizal Feb 8, 2017

stacey-gammon Feb 8, 2017

uboness Feb 15, 2017

spalger left a comment

cjcenizal left a comment

weltenwort left a comment

weltenwort Feb 9, 2017

stacey-gammon Feb 9, 2017

spalger Feb 9, 2017

weltenwort left a comment

stacey-gammon commented Feb 9, 2017

spalger Feb 9, 2017

stacey-gammon Feb 9, 2017

weltenwort Feb 9, 2017

spalger Feb 9, 2017

thomasneirynck commented Feb 9, 2017

stacey-gammon commented Feb 13, 2017

kimjoar Feb 13, 2017

uboness Feb 13, 2017

kimjoar Feb 13, 2017

uboness Feb 13, 2017

stacey-gammon Feb 13, 2017

uboness Feb 15, 2017

epixa Feb 15, 2017

uboness Feb 15, 2017

epixa Feb 15, 2017

Bargs Feb 15, 2017

Introducing react icons! #10247

Introducing react icons! #10247

Conversation

stacey-gammon commented Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger left a comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

weltenwort 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

weltenwort left a comment

Choose a reason for hiding this comment

stacey-gammon commented Feb 9, 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

thomasneirynck commented Feb 9, 2017

stacey-gammon commented Feb 13, 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

Choose a reason for hiding this comment

stacey-gammon commented Feb 8, 2017 •

edited

Loading