Adding table_info component #11778
Adding table_info component #11778
Conversation
|
@cjcenizal When you see this component in action on http://localhost:5601/app/kibana#/testbed , you'll notice that there is no top border shown. Is there some way to fix this? EDIT: If it helps, |
|
(with the exception of the failing build) |
|
i want all the gifs please |
|
Won't somebody (@ycombinator) please just give @lukasolson the gifs!? |
|
GIFs are coming, just working on other stuff (fixing the build) at the moment. |
| @@ -5,6 +5,10 @@ | |||
| you need to demonstate your functionality. Nothing need be preserved. --> | |||
| <!-- CONTENT START --> | |||
|
|
|||
| <div class="kuiControlledTable"> | |||
| <loading-results results-name="all the things"></loading-results> | |||
There was a problem hiding this comment.
In my experience, there's little benefit to tightly controlling a component's input like this, because unanticipated edge cases tend to pop up. For example, internationalization becomes more difficult because you typically need to translate entire phrases instead of substrings of a phrase. How about we change this interface to be a little more compositional:
<loading-results>
Loading all the things…
</loading-results>This will mirror how I'd implement this component in React.
There was a problem hiding this comment.
Good point about how the current design doesn't play well with internationalization. I like the approach you are proposing but it then begs the question: should loading-results and no-items really be a single component, something like items-message or something else (naming is hard!)?
There was a problem hiding this comment.
You're absolutely right... we should combine these into a single component. Not sure why I made it two in the first place! How about kuiTableInfo?
There was a problem hiding this comment.
So, just so I'm clear, I'm going to do the following as part of this PR:
- rename this directive from
loadingResultstotableInfo, - rename the
kuiLoadingItemsclass in the UI Framework tokuiTableInfo, - get rid of the
kuiNoItemsclass in the UI Framework, - update all usages affected by the above changes
| template: template, | ||
| scope: { | ||
| resultsName: '@', | ||
| }, |
There was a problem hiding this comment.
To implement my suggestion, we'd have to add transclude: true in here somewhere.
|
@ycombinator In terms of the missing top border, that's technically correct since the top border is deliberately missing in the UI Framework. This is to make this component play nicely with the ToolBar component. I'd like to deprecate the ToolBar component and add a top border to LoadingResults and NoItems in the future. Where are you seeing |
cjcenizal
added
the
Team:Platform-Design
ycombinator
changed the title
|
@cjcenizal I've made the changes you requested. Can you take another gander at this PR, please? Thanks! |
ui_framework/dist/ui_framework.css
Outdated
| @@ -2239,7 +2239,7 @@ body { | |||
| .kuiModalFooter > * + * { | |||
| margin-left: 5px; } | |||
|
|
|||
| .kuiNoItems { | |||
| .kuiTableInfo { | |||
There was a problem hiding this comment.
I think you may have done a find/replace to change these, but this is a generated filed. So we'll need to update ui_framework/components/index.scss to remove the original component imports and add the new component import. We just need to change lines 21 through 53 to be this:
// Components
@import "action_item/index";
@import "badge/index";
@import "bar/index";
@import "button/index";
@import "card/index";
@import "collapse_button/index";
@import "column/index";
@import "event/index";
@import "form/index";
@import "form_layout/index";
@import "gallery/index";
@import "header_bar/index";
@import "icon/index";
@import "info_panel/index";
@import "link/index";
@import "local_nav/index";
@import "menu/index";
@import "menu_button/index";
@import "micro_button/index";
@import "modal/index";
@import "notice/index";
@import "panel/index";
@import "prompt_for_items/index";
@import "status_text/index";
@import "table/index";
@import "table_info/index";
@import "tabs/index";
@import "toggle_button/index";
@import "tool_bar/index";
@import "typography/index";
@import "vertical_rhythm/index";
@import "view/index";There was a problem hiding this comment.
Ugh, good catch! Will fix.
|
@cjcenizal I fixed the imports and regenerated the CSS. Please take another look when you have a chance. Thanks! |
|
jenkins, test this |
ycombinator
force-pushed
the
promote-components-to-oss/loading-results
branch
from
58e3b08 to
aeca54d
Compare
* Adding loading-results component * Adding example usage on testbed * Fixing module name * Adding kuiControlledTable container * Replacing kuiNoItems and kuiLoadingResults with kuiTableInfo * Making directive compositional * Fixing imports * Re-generating fixed CSS
|
Backported to:
|
* Adding loading-results component * Adding example usage on testbed * Fixing module name * Adding kuiControlledTable container * Replacing kuiNoItems and kuiLoadingResults with kuiTableInfo * Making directive compositional * Fixing imports * Re-generating fixed CSS
* Adding loading-results component * Adding example usage on testbed * Fixing module name * Adding kuiControlledTable container * Replacing kuiNoItems and kuiLoadingResults with kuiTableInfo * Making directive compositional * Fixing imports * Re-generating fixed CSS
This PR adds a
table-infocomponent, intended to be used inside akuiControlledTablecontainer, like so:Screenshot
Visit http://localhost:5601/app/kibana#/testbed to see it in action.