[ui] Rework of node/job attributes/meta using PathTree #23290
Conversation
Ember Test Audit comparison
|
philrenaud
force-pushed
the
16034-ui-bug-node-metadata-doesnt-show-up-in-the-ui-when-key-is-also-a-prefix-for-others
branch
from
a61fe30 to
7b922a9
Compare
philrenaud
force-pushed
the
16034-ui-bug-node-metadata-doesnt-show-up-in-the-ui-when-key-is-also-a-prefix-for-others
branch
from
7b922a9 to
38326a8
Compare
| @@ -793,7 +793,7 @@ | |||
| {{capitalize a.item.name}} | |||
| Attributes | |||
| </div> | |||
| {{#if a.item.attributes.structured}} | |||
| {{#if a.item.attributesShort}} | |||
There was a problem hiding this comment.
The conditional here now matches the property we use, rather than being a proxy for it.
| @editable={{@editable}} | ||
| @onKVSave={{@onKVSave}} | ||
| @copyable={{@copyable}} | ||
| /> |
There was a problem hiding this comment.
Two notable changes here:
- We no longer show the "key-prefix-only" blue headers like we used to. These were
colspan="2"/full-width rows that both broke table accessibility conventions and made the page taller than it needed to be (especially with a lot of client metadata) - we place "terminal" items first, and then nested ones below, kind of like a filesystem layout might look in an explorer window, rather than alphabetizing all prefixes. I am less attached to this change but it feels right so far.
| @@ -39,11 +39,14 @@ export default class PathTree { | |||
| /** | |||
| * @param {MutableArray<VariableModel>} variables | |||
| */ | |||
| constructor(variables) { | |||
| constructor(variables, { delimiter = '/' } = {}) { | |||
There was a problem hiding this comment.
I stopped short of fully generifying this (changing .files[] to .terminal[] etc.) but did:
- parameterize the delimiter
- add
prefixas a returned field for ease of use in the UI
| @@ -81,27 +92,4 @@ module('Integration | Component | attributes table', function (hooks) { | |||
| 'properties' | |||
| ); | |||
| }); | |||
|
|
|||
| test('should render a row for key/value pairs even when the value is another object', async function (assert) { | |||
There was a problem hiding this comment.
Test became obsolete when we stopped showing the blue "key" rows.
There was a problem hiding this comment.
LGTM! In addition to reviewing the code as best I can, I also took a look through the UI preview build and everything looks as I'd expect.
One note which is probably out of scope for this PR: the faded-out prefix is kinda low-contrast relative to the background. In places like Node attributes, where we don't show a "tree", this means that we never display the prefix value as black text. Snippet of what I mean from here:
Here "kernel" is never being shown to the user as black text, only faded.
|
@tgross your comment got me to spitball with some components a bit; what would you think about the look of metadata/attributes if they were set in our Is this too visually heavy for what it is? |
Yeah, feels heavy to me. |
philrenaud
force-pushed
the
16034-ui-bug-node-metadata-doesnt-show-up-in-the-ui-when-key-is-also-a-prefix-for-others
branch
from
145e13b to
d318131
Compare
philrenaud
added
backport/ent/1.8.x+ent
philrenaud
removed
the
backport/ent/1.8.x+ent
philrenaud
deleted the
16034-ui-bug-node-metadata-doesnt-show-up-in-the-ui-when-key-is-also-a-prefix-for-others
branch
This extends and reuses PathTree logic originally written for Variables in #13202
This PR specifically fixes a bug where a metadata k/v item wouldn't show up in the UI if its full key was the prefix of a nested child key.
For example, this metadata:
would produce the following:
and now produces:
