Add EUI Framework as a dependency #14948
Conversation
cjcenizal
added
Team:Platform-Design
| { | ||
| markup: field.searchable ? yesTemplate : noTemplate, | ||
| }, { | ||
| // TODO: What is this? |
| @@ -16,6 +16,7 @@ module.directive('kbnRows', function ($compile, $rootScope, getAppState, Private | |||
| return $(document.createElement('td')); | |||
| } | |||
|
|
|||
| // TODO: Reimplement this in React. | |||
| @@ -39,6 +40,7 @@ module.directive('kbnRows', function ($compile, $rootScope, getAppState, Private | |||
| let $cell; | |||
| let $cellContent; | |||
|
|
|||
| // TODO: Reimplement this in React. | |||
| @@ -60,6 +62,8 @@ module.directive('kbnRows', function ($compile, $rootScope, getAppState, Private | |||
|
|
|||
| // TODO: It would be better to actually check the type of the field, but we don't have | |||
| // access to it here. This may become a problem with the switch to BigNumber | |||
|
|
|||
| // TODO: Reimplement this in React. | |||
|
If merged, this PR fixes #11902 |
… table. Deleted a bunch of stuff which may actually be important.
cjcenizal
force-pushed
the
eui-dependency
branch
from
a57d3dc
to
15a47a7
Compare
| render: () => { | ||
| let content; | ||
| if (filter === this.editing) { | ||
| // TODO: Ensure value changes during onChange. |
| </button> | ||
| ); | ||
| } else { | ||
| // TODO: Are these onClicks handled correctly? Do we depend upon |
Talked in person about this, but I'd advise against this (CJ mostly agreed). I'd rather have things not look perfect short-term than deal with individual overwrites and tokenization. Part of EUI's strength right now is that is actually has so little variables. I don't want to introduce complexity to the code just because current Kibana itself has faults. It will make the mental juggling difficult. Stick to global variables.
I'd rather EUI was installed natively, and the "theme" / compile step existed in the Kibana code itself. This is more akin to how we'd ask any other consumer to do it. At the end of the day, the theme vars can all be in one file. I don't think we should consume EUI CSS, but its sass (this allows us to write Kibana specific sass through importing / extending EUI). Again this is similar to how cloud or anyone else would consume it. I'd see it as something like this... // Kibana_k7.scss
@import kibana/kibana_theme_variables
@import eui/global_styling/core
@import eui/components/index
@import kibana/components/index // This is great, cuz we're now using EUI vars here and can depreciate the less whenever we want. We could even convert the less files and put it here so we can use the same variable scope.
@import kibana/k6_shims_we_might_need_and_can_later_throwawayOur load order would then look something like this... And if we coverted the current less to sass it could even be this... And then finally...
Think it is important to call out this is a partial audit. EUI requires a reset / sane CSS starting point. Since Kibana currently doesn't have one, introducing that on-top will introduce visual breaks in CURRENT kibana AND in EUI that is installed in Kibana. With the sprawl of code as it is, this will take a decent portion of time to address. Not saying we shouldn't do it, but it is work dedicated to fixing old stuff vs. building new stuff.
I think you're implying it, but I'd go one step further and say we need engineers to not just convert stuff to the new design that we provide, but also help build the core components of EUI. We shouldn't rely on CJ to build every complex UI component we need. Examples: combo box, date picker, toolips, filter / query bar...etc. Everyone uses these and we need more people involved in building them, otherwise we're blocked.
Taking this farther, the K6 stuff should be the easiest code to replace. We should prioritize porting the bits we still need to pull over (color picker...etc) and very quickly replace anything in use with its EUI equivalent. People will always copy/paste out of the current codebase thinking "well, it's used over here...". |
++ I updated the description to reflect your stance. Thanks for the reminder!
My initial preference is to punt on this question and take the simplest route to make progress right now. One benefit to keeping theme vars in EUI makes it easier to test them in the browser with the docs site. Do we solve a pressing problem by moving the variables into Kibana and adding a compilation step?
I agree with all of your other points! Thanks for bringing them up man. |
It means that Kibana can use EUI vars. Which means we've suddenly removed a lot of complexity from our codebase. We go from 27 colors of gray (and then more spread across who knows how many files) down to the 5 we use in EUI. It means we can actually solve the Dark Mode problem (which we should probably add to your list of problems). It also means we simplify our compile. We could move it down to one file for all active development, vs. the two needed (one manually!) right now. Dunno. Seems really easy for us to do compared to everything else you ahve here, and has very nice upsides. It also keeps EUI very clean and easier to manage. |
|
@snide Just to clarify, for us to use EUI vars in Kibana would mean converting our LESS into SCSS and then replacing our use of our existing vars with the EUI vars, right? This would take some work to go through the existing LESS code, which is code we're planning on removing. OTOH I think some benefits to this proposal are that it would help our look-and-feel be more consistent because we're using a small, well-defined color palette, and it would make it easy for you to change the design by changing individual hex values in that palette which would then update the entire UI. How would it solve the Dark Theme problem? (I added this to our list of problems btw, good call!) Does the solution require us to add a feature for toggling between light and dark themes globally? I was thinking we could compile the dark theme SCSS nested beneath a |
I guess I see the less to sass conversion as pretty simple (from a time perspective) so why not? Separately you're right that converting the code itself to use the vars would take longer, but hey, at least we can do that gradually and everyone is writing in the same language in a known load order. The alternative is that we maintain two worlds, forever separated, until we eventually kill the less off... a long time from now, hoping that no one adds to it? I guess I'd throw it back and say why wouldn't we do it? What are the downsides of moving the less over to sass today, even without the var conversion? Only downside that I can think of is we'd lose a day or two in the beginning to changing the build system (not that I'd know how to do that, I'm just assuming)?
For dark mode, i don't think it would require us to toggle it globally, but we should definitely move away from the nesting system we use now, as it's pretty brittle (you get competing cascades because only one version has the extra selector so Also, kind of separate, but I'd really love to get local Kibana to auto-refresh on CSS changes. Working in it now is extremely painful with of all the reloads. We should ask for some help from ops to make that happen if we move forward with a large CSS project like this. |
|
I don't have strong opinions about the fundamental approach we take here, but I want to clarify that we probably will need individual overrides and stuff in Kibana 6.x. They could be hacky cascading CSS things rather than tying into eui for all I care, but we can't ship stuff that know to be broken to people. We might be targeting this new design for release about a year from now, but we'll be maintaining 6.x for the next 2 years, and some users will end up living with 6.x for even longer than that. This means more work for us, but it's just reality of providing software to people rather than running our own service. In the grand scheme of things, this isn't anything new to Kibana. We already deal with the consequences of maintaining overrides every day. This brings me to my next point: EUI is going to need version branches that coincide with Kibana's versions eventually. At any given time, we have 4 different version branches being actively maintained in Kibana, and each of them have restrictions on what sort of changes can go in. We need to be able to make minor-version level changes to EUI for the next minors, breaking changes for the next major, and patch level bug fixes for both the legacy and current version lines all at the same time. This may be something we don't need to deal with until closer to 7.0 launch, but I figured I'd bring it up anyway since the idea of having a 6.x branch for EUI has been brought up in a couple of conversations in the last few months, and each time it has mostly been dismissed. Unrelated, we need to figure out how we're going to do this sass/css thing. Converting less to sass doesn't seem anywhere near as complicated to me as updating Kibana to deal with sass in dev mode, building all sass in the build step, and then dealing with only css in production builds. We can't ship sass with Kibana because it's a native module. |
|
@cjcenizal If we chose not to convert existing less to sass right now, that is something we could explore down the line if it was causing us issues, right? |
|
@epixa Absolutely. |
I think its a good idea to treat EUI the same as everything else in Elastic. We should make the current version 6.x and distribute it as linked version wize our other codebases. Where I differ is that I don't think we need to have much Kibana in EUI (a Kibana branch per se), but should instead import EUI into Kibana and extend it as needed. This makes it much easier for Kibana to "add hacks" as you bring up, without messing with the core system.
I can't think of a good reason why we need to stick with sass if this is such a blocker. There's nothing we're gaining on the development side using Less over Sass other than it being a more popular standard. They are virtually the same from a feature perspective. What I do think is important, is that we use the same language, whichever it may be, so that the system is coherent and usable, even during transition. This benefits everyone. Remember too, our current site uses a lot of sass, so this already is an existing problem, not a new one being introduced. If we want to convert our sass to less, that's fine with me. I just want the code talking to each other. Without doing something like this we're saying that to edit something global, we will need to touch 4 independent systems to make a change. The K7 Sass, the K6 sass, the K6 less and the Xpack Less. |
I think we're completely on the same page here.
Come the new platform being completely rolled out, which hopefully will be in 7.0, we don't want any compilation to happen in Kibana production builds even if it is technically possible. In other words, we don't want to compile less either. The build step in Kibana will actually run the relevant built tasks to compile down whatever we choose into css, and we'll ship the resulting css. In other words, I think this is a problem we need to address regardless. I wasn't bringing it up as a problem unique to this PR, it's just something we need to sort out in any case. Unless your plan was to write the component framework in raw css, which I don't think is a good idea. Technically speaking, using less would be more straight forward in Kibana right now since we don't need to change any build system. You just add the less files and ago, and then the whole build change for the new platform becomes a new platform problem. But come 7.0, we have the same restrictions regardless of language choice, so let's choose the one we want now and figure out how to make it work in the meantime.
To clarify, this is one more place than we're doing today, right? I like the idea of getting rid of the existing component framework in favor of EUI as rapidly as possible. That would get rid of one of those steps pretty quickly. At that point, we're back into the same situation that we're in today, where we have 3 "places" to consider when making certain types of changes. Up until this conversation, I think we've all been pretty much OK with that as a short term reality throughout 6.x, so it's not necessarily something we must solve right now unless it does turn out to be worse than we have been anticipating. But at that time we can just convert from less to sass, and we may have a good deal less less (no pun intended) to convert as more and more stuff has been switched over to unmodified EUI. This is assuming, of course, that my original assertion here is correct - that adding EUI sass is only adding a fourth "place" to touch rather than some fundamentally new challenge. |
|
The problem is it makes it really hard for me to design and work rapidly. I should make a video, but think of something like what I did for Kibana 6. That was only a thousand lines of code, but it took me a month, because of how scattered the system is (with 3 systems) and how long it takes for Kibana to actually refresh after I make a change. let's say I want to change blue when we do this. I will have to...
What I'd prefer doing is... convert ALL of our preprocessing to one language. Not change vars immediately, just convert it. Then I'd have them import each other into one Kibana.whatever file so that they have an actual inheritance. I'd then trigger live reload on CSS changes. This makes it more manageable and I could now:
I can slowly fix it, rather than replace it outright. |
|
@snide Will you need to change colors globally very often? I was hoping we'd spend most of our time replacing the old LESS styles, raw markup, and old UI Framework components with EUI components. End result being a reduced amount of old LESS we'd have to deal with, akin to what Court said. I agree that our workflow needs to be improved. I can't even get the symlinked EUI dependency to update when I rebuild the CSS. I think I had it working originally but now that I've unlinked and relinked EUI, updates don't show up. We need this workflow to be as quick as possible. If we have to, we can build a temporary tool that watches the compiled CSS for changes and just copies it to where it needs to be. I do think converting our codebase from Sass to LESS or vice versa will be a challenge. Based on what I understand from the docs, the languages don't have feature parity. For example, I'd like to know how our pattern for building Buttons in Sass will be implemented in Less, where loops and conditionals are implemented very differently |
|
I couldn’t stop thinking about version management after @epixa mentioned it. After chewing it over, I think we need to optimize for sharing code between versions of Kibana instead of sharing code between Kibana and other web properties. Sharing code between versions consists of:
If our EUI dependency lives as an external repo then we’ll have to create an EUI branch to correspond with each version branch of Kibana (e.g. 6.x, 6.1). Every change to EUI will require cutting a new tag to represent this “EUI version” within the particular Kibana version (e.g. 6.2.0-0.0.1). If this change needs to be backported, we’ll backport it to the relevant Kibaba version branches and cut new tags (e.g. 6.1.1-14.0.8). Then, in Kibana, we’ll update the NPM dependency, re-install it, and update our UI code to consume the changes correctly. We can backport these changes, but it won’t be a clean backport — we’ll need to manually update the package.json to depend upon the EUI version which contains the backported EUI changes. And we’ll probably need to test locally and wait for the CI to be green to ensure the EUI backport was correct and that we didn’t make a typo when editing package.json. If our EUI dependency lives within Kibana, then the backporting process is the same as it is today. You backport your change, ensure there aren’t any merge conflicts, and then merge it. I prefer this option because it’s simpler and less error-prone. If we choose to go this route then we can migrate the EUI code into the Kibana repo and the two frameworks can live side-by-side, e.g. as ui_framework_kibana and ui_framework_elastic directories. At some point we can look into sharing code with the EUI repo so they can stay in sync. Some of the questions I’d like to answer when we discuss this PR as a whole are:
|
You can still change the language and build order before you do componentization. I don't see why it would be a one or the other proposition. I'm suggesting that before we do the long slog of replacing everything, we take a couple days to simplify our build system so that we can make the legacy code easier to work with (by making the language the same and building a single CSS file). It makes our life easier in the 6.x world. What downsides am I missing in this? Would it take significant amount of time to change the language alone and have the files import into one file? As a basic example. Since we're going to be living in two worlds for a long time, I can go through and organize and replace a lot of legacy Kibana. If it's in one load order I can do that through one variable scope with one css file to debug. I can also affect the actual load order, versus wondering how our weird build system parses our directory structure to construct, then merge, disparate files. Our current method of throwing CSS on top with overwrites is difficult to manage.
Fair warning. I'm a novice here, so patience if I'm missing something dumb. Why can't EUI version against the Elastic schedule and we just make that a thing? Example (We make 6.2 the next release of EUI). Kibana's 6.2 release inherits 6.2.x EUI. Want to backport? Just backport to the same named release. Kibana shouldn't have to do anything. Feels like a fairly painless way to manage this and not require any hairy dependency management. Tests can still run in the background as they always do with locked (but backported / updated versions). If we figured this out between all our various properties already I don't see why we can't do it for a (relatively simpler) UI library.
Do you have any other worries with keeping the library collaborative between teams? Is it just versioning? That seems like something we can solve. I agree with you the last thing any of us wants to do is maintain mirror systems (which seems painful enough with k6/k7). Working with the other teams seems to be pretty positive (look at the last month's combined output!). I'd be really bummed if we had to give all that up. |
I think I have a better understanding of how this change would help you now. Thanks for explaining. I think the major blocker is technical, which @epixa or someone on the Operations team could shed more light on. My understanding is that our optimize process will run on the consumer's machine when we ship a production build of Kibana -- this process would use
Are you suggesting that for each Kibana branch we have (master, 6.x, 6.1), we have a corresponding branch in EUI which we cut releases from? So, Kibana 6.x has a dependency upon the EUI 6.x branch. As changes are backported to EUI 6.x they're available to Kibana 6.x once you reinstall the dependency. When we cut v6.1.0 of the stack, we create an EUI v6.1.0 release and update Kibana 6.x to depend upon that version before we create the Kibana v6.1.0 build. I think this works for the most part but the manual step when we create a release seems like a stumbling block. I wonder if we can fix that? I think the only other problem I have with this is that there's no way to do forensics on the changes throughout the course of development of a particular release. For example, if you check out an older commit in Kibana, the dependency upon EUI will still be pointing to a branch in active development. So as time goes on, the Kibana represented by that commit will change based on the changes in EUI, which feels unexpected to me. Anyone else have thoughts on this?
Yes, my only concern is optimizing for sharing code between versions over sharing it between teams. I feel the same as you do in that I also think that moving EUI to an external repo has yielded many benefits and I'd prefer not to duplicate codebases. But if we have to give them up (hopefully temporarily) in exchange for a faster and stabler Kibana dev process, then I think it's a worthwhile tradeoff. |
|
Outcome of this discussion recorded at #15282. |
Goal
The goal is to allow Kibana engineers to build user interfaces in React using the EUI Framework (previously known as the K7 UI Framework) components.
elastic/eui#153 updates the EUI Framework with a K6 theme. Check that PR out for more details on what this means from a EUI Framework perspective.
Changes in this PR
paginated_table.jsdirective in React, using EUI components.Experiment
As an experiment, I converted the Management landing page and the
paginated_table.jsdirective (used by the Index Fields, Scripted Fields, and Source Filters tables) to React and reimplemented them using EUI components.Before
After (K6 theme switched on)
After (K6 theme switched off)
Observations
Look and feel
Styles
Instead of using mixins likeTalked this over with Dave and I agreed to try his approach of avoiding this, for reasons he's stated below.@include euiFontSizeXS, we should probably create component-specific vars for various font-sizes, and set them to the specific EUI font-size variable with!defaultso we can override them in the theme. This would allow me to tweak theEuiTablestyles without directly editing the SCSS.File size
Technical debt
Problems
EUI icons aren't imported/available.Fixed by Use inline SVGs for icons eui#160TODOin this PR.Proposed plan
This PR has shown that it’s technically feasible to use EUI components in the Kibana UI without causing problems from a design standpoint. I propose that our next steps be:
Partial audit
Here are a few of the visual regressions incurred by the CSS reset changes.
Notifications
Notifications are missing padding around their edges, causing text to appear misaligned vertically and buttons to be flush against the top and bottom edges.
Timepicker
aelements with nohrefattribute.Query bar
The "Add filter" button doesn't have a pointer cursor when hovered.
Discover
In "Single document" view, the tabs lack pointer cursors when hovered, possibly because they're
aelements withouthrefattributes.Note: this applies to the tabs in an expanded row of the doc table, too.
Visualize
The sidebar tabs lack pointer cursors when hovered, possibly because they're
aelements withouthrefattributes.The spacing between "Select bucket types" and the menu is too tight.