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.

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.

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);

coooool

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

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

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

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.