[Lens] Don't block render on missing field #149262

drewdaemon · 2023-01-19T20:19:18Z

Summary

Resolve #143673
Resolve #147485

This PR introduces the new error/warning UI outlined in #147485. In addition

Missing fields are now treated as non-blocking errors ([Lens] Don't block render on missing field validation errors #143673) and are shown in the toolbar, the dimension button, and the embeddable
Precision warnings are now shown on the corresponding dimension button as well as in the editor toolbar. I included this so that there is an example of warnings on dimension buttons to see.

Screenshots

Editor

Errors

Warnings

Both

Embeddable

Errors

Warnings

Both

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios (flaky runner in progress).
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)

…ck-render-on-missing-field

…fix'

…m:andrewctate/kibana into 143673/dont-block-render-on-missing-field

MichaelMarcialis · 2023-02-02T17:29:09Z

@drewdaemon: Thanks for those comments. Some quick responses below:

the EuiCode component wraps text in a code element, which wouldn't be semantically correct in this instance (potential accessibility issue).

I verified that the tooltip text was accessible to a screenreader, however the rest of your reasons for preferring bold typography make sense.

Thanks for checking! In this case though, my accessibility concern was more about the semantics of the element and that the string would be presented as a chunk of code to users, when it's actually not code.

When using the clientip field from the Kibana Sample Data Logs data view to produce the Top 3 values of clientip might be an approximation. warning, I noticed that the warning wasn't showing up when the visualization was placed in a dashboard. Is that intentional?

This warning wasn't shown on the dashboard before and I didn't change that. We can decide to show it, but just be aware that it's a change in behavior. WDYT?

Interesting. Ultimately, I'll defer to the product folks on this. To my mind though, unless the warning doesn't make sense in the context of a dashboard or embed, I think it would make sense to include it (minus the corrective action button, since the user is outside the editor at that point). CCing @ninoslavmiskovic, in case he has an opinion on it.

I had one additional last-minute thought. Originally, I was keen on the notion of having the error/warning icons replace any other prepended icons that were shown on the related dimension item (color swatch, swatch with slash, collapse, hidden, etc.), as you've shown here in your PR. Upon further reflection and after testing, I'm now thinking it may be too heavy-handed for us to replace those icons outright (as some meta information about the dimension may be lost in the process in some instances. Instead, could we show both, but have it so the error/warning icons appear adjacent to the text (rather than sandwiching the other icon in between), as they share the same color? Example below:

I don't think this is critical though, so if you'd prefer this to be spun off as a separate issue, let me know.

drewdaemon · 2023-02-02T18:08:04Z

@MichaelMarcialis

Instead, could we show both, but have it so the error/warning icons appear adjacent to the text (rather than sandwiching the other icon in between), as they share the same color?

Done. This is what it looks like

WDYT?

drewdaemon · 2023-02-02T18:11:30Z

@MichaelMarcialis also, here's the new dimension button styling with a color palette dimension

When the delete button is hovered, it runs into the color palette indicator. I'll leave it to you to decide whether that's an issue.

ninoslavmiskovic · 2023-02-02T20:00:22Z

A couple of comments @MichaelMarcialis and @drewdaemon :

I think it is important to showcase to the user that he/she should focus on the warning, and therefore it make sense to overwrite the existing icon to nudge the user to hover over the warning and figure out what is wrong.
Adding to icons does not feels scalable - What if we want to add an "i" icon in the future for information etc.

I have in similar indicator exercises gotten lucky and used text and the field border to indicate something is wrong. Red text + red border. Or even making the background of the form field red.

drewdaemon · 2023-02-02T20:23:44Z

@MichaelMarcialis here are the icons with the new padding.

@ninoslavmiskovic thanks for your feedback. I believe @MichaelMarcialis will be out for the first of next week, so if you don't mind, I think we can take your objections to the dimension button icon pattern as a follow up discussion. It's easily changed—if we decide to go another route it can make it in for 8.8 or even sneak in as a bug fix after feature freeze for 8.7 if need be.

With feature freeze looming, I don't want to hold up this PR.

MichaelMarcialis · 2023-02-02T20:35:32Z

A couple of comments @MichaelMarcialis and @drewdaemon :

I think it is important to showcase to the user that he/she should focus on the warning, and therefore it make sense to overwrite the existing icon to nudge the user to hover over the warning and figure out what is wrong.

Adding to icons does not feels scalable - What if we want to add an "i" icon in the future for information etc.

I have in similar indicator exercises gotten lucky and used text and the field border to indicate something is wrong. Red text + red border. Or even making the background of the form field red.

@ninoslavmiskovic: For dimension items specifically, I would try to avoid altering the border or background color in an error or warning state (at least, not without some other stylistic changes first). The border styling is currently being used to on hover to indicate drag-and-drop. The changing the background color also becomes problematic/conflicting when you take into account the other elements of color already being used in the dimension items (color swatches, color palettes, delete button, etc.). Not saying we can't do these things, but we'd need to make other adjustments to accommodate.

As for retaining the swatch/visibility/collapse icon before the error/warning icon, I'll ultimately defer to you on that. I agree with your scalability concerns. I was worried that the user may lose some context about the dimension in the process, but if you feel that loss is not significant or important, we can just have the error/warning icons overtake those other icon prepends as @drewdaemon previously had it. As he mentioned, I'm out Monday and Tuesday next week, so I'm good with whatever ya'll decide there.

MichaelMarcialis

Thanks for making those changes, @drewdaemon! I've left some small comments below for my re-review, but nothing worth blocking you on. I'll go ahead and approve now, as I'll be out early next week. Great work here.

As for the outstanding questions on how to handle the prepended icons and whether to show that warning I mentioned previous on dashboards/embeds, I'll defer to whatever you and the team decide.

MichaelMarcialis · 2023-02-02T20:58:28Z

...plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_button_icon.tsx

+      <EuiIcon
+        {...baseIconProps}
+        type="color"
+        color="text"


Can we change this to subdued to match the other prepend icons?

Suggested change

color="text"

color="subdued"

MichaelMarcialis · 2023-02-02T21:00:13Z

...gins/lens/public/editor_frame_service/editor_frame/config_panel/buttons/dimension_button.tsx

      <EuiButtonIcon
        className="lnsLayerPanel__dimensionRemove"
        data-test-subj="indexPattern-dimension-remove"
-        iconType="cross"
-        iconSize="s"
+        iconType="trash"
        size="s"


Possible to reduce the size here to xs to fit more comfortably in the dimension item box?

Suggested change

size="s"

size="xs"

MichaelMarcialis · 2023-02-02T21:07:59Z

...gins/lens/public/editor_frame_service/editor_frame/config_panel/buttons/dimension_button.tsx

-      >
-        <ColorIndicator accessorConfig={accessorConfig}>{children}</ColorIndicator>
-      </EuiLink>
+      <EuiFlexItem>


Is EuiFlexItem needed here? May be able to replace with a regular old div.

MichaelMarcialis · 2023-02-02T21:12:13Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/message_list.tsx

+            {errorCount > 0 && (
+              <>
+                <EuiIcon type={IconError} />
+                {errorCount}
+              </>
+            )}
+            {warningCount > 0 && (
+              <>
+                <EuiIcon
+                  type={IconWarning}
+                  css={css`
+                    margin-left: 4px;
+                  `}
+                />
+                {warningCount}
+              </>
+            )}


Would it be possible to give the error and warning icons a prop of size="s" when used on dashboard panels and embeds (but not in the Lens toolbar) to better match the designs?

kibana-ci · 2023-02-02T22:52:20Z

💚 Build Succeeded

Buildkite Build
Commit: e5c8042

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	890	891	+1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`lens`	599	597	-2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	1.3MB	1.3MB	+3.6KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`data`	407.3KB	407.0KB	-310.0B

Unknown metric groups

API count

id	before	after	diff
`lens`	694	692	-2

History

💔 Build #105281 failed a7779ba
💔 Build #105241 failed ee10bec
💔 Build #104935 failed f49046c
💔 Build #104919 failed 6fb21d0
💚 Build #104843 succeeded f06a868
💔 Build #104810 failed 0f2b453

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

drewdaemon · 2023-02-03T00:13:34Z

@MichaelMarcialis I've merged the PR with the understanding that I will address your final round of feedback in #150241

drewdaemon and others added 30 commits December 16, 2022 13:14

datasource errors become user messages

8569506

unified search errors become user messages

f160f97

datasource warnings become user messages

6f634b9

datasource deprecation warnings become user messages

7c03428

visualization error messages are user messages

5bf0124

visualization warnings become user messages

bf9021e

clean up some remaining usages

3933fe8

listen for messages from the active visualization

7e51cd8

unknown visualization error becomes user message

843c6d5

missing index pattern error becomes user message

71adbe8

update tests

0de8d2c

clean up workspace panel error logic

b406b44

search warnings become user messages

e0ba1ce

expression parse errors become user messages

8f3afc6

gauge: dimension errors become user messages

6a5e7bf

xy: group invalid message becomes user message

fdd320d

remove invalid and invalid message props from the group config

147ad01

Merge branch 'main' of github.com:elastic/kibana into 143673/dont-blo…

1fabfd5

…ck-render-on-missing-field

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

6d638b0

…fix'

user messages come to the embeddable

e542595

embeddable search warnings become user messages

ef8661c

Merge branch '143673/dont-block-render-on-missing-field' of github.co…

c49a210

…m:andrewctate/kibana into 143673/dont-block-render-on-missing-field

progress toward user messages in embeddable

64eee6a

unify user message query interface

89ffe66

remove all error message generation from docToExpression

bec1570

utilize existing index patterns cache

23077ed

fix infinite loop scenario

8701293

re-enable filtering out invalid suggestion configurations

862c44c

fix some types

39cfd6e

fix types and i18n

dd20547

drewdaemon added 2 commits February 2, 2023 11:16

update translations

c745c5a

update formula test

cdcb7a6

drewdaemon added 3 commits February 2, 2023 11:46

use EuiButtonIcon for dimension button trash can

0042b96

update dimension button iconography styling

24d6b96

show both error status and meta info icons on dimension button

a336602

drewdaemon added 3 commits February 2, 2023 12:17

show missing sort field error in workspace

5765bcf

replace code highlighting with bolding

ee10bec

update jest test

3d5f745

extra margin between warning icon and error count

a7779ba

drewdaemon requested a review from MichaelMarcialis February 2, 2023 20:23

update snapshots

e5c8042

MichaelMarcialis approved these changes Feb 2, 2023

View reviewed changes

drewdaemon merged commit be37fa1 into elastic:main Feb 3, 2023

kibanamachine added v8.7.0 backport:skip This commit does not require backporting labels Feb 3, 2023

drewdaemon mentioned this pull request Feb 3, 2023

[Lens] dimension button design follow-ups #150241

Closed

drewdaemon mentioned this pull request Feb 6, 2023

[Lens] don't block render on missing sort field #150356

Merged

drewdaemon mentioned this pull request Mar 21, 2023

[Lens] Dimension Item Expanded Drop Zones Mis-Styled #153371

Closed

drewdaemon mentioned this pull request Sep 26, 2023

[Lens] allow rendering despite invalid dimensions/layers #144906

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Don't block render on missing field #149262

[Lens] Don't block render on missing field #149262

drewdaemon commented Jan 19, 2023 •

edited

Loading

MichaelMarcialis commented Feb 2, 2023

drewdaemon commented Feb 2, 2023

drewdaemon commented Feb 2, 2023

ninoslavmiskovic commented Feb 2, 2023

drewdaemon commented Feb 2, 2023

MichaelMarcialis commented Feb 2, 2023

MichaelMarcialis left a comment

MichaelMarcialis Feb 2, 2023

MichaelMarcialis Feb 2, 2023

MichaelMarcialis Feb 2, 2023

MichaelMarcialis Feb 2, 2023

kibana-ci commented Feb 2, 2023

API count

drewdaemon commented Feb 3, 2023

[Lens] Don't block render on missing field #149262

[Lens] Don't block render on missing field #149262

Conversation

drewdaemon commented Jan 19, 2023 • edited Loading

Summary

Screenshots

Editor

Errors

Warnings

Both

Embeddable

Errors

Warnings

Both

Checklist

MichaelMarcialis commented Feb 2, 2023

drewdaemon commented Feb 2, 2023

drewdaemon commented Feb 2, 2023

ninoslavmiskovic commented Feb 2, 2023

drewdaemon commented Feb 2, 2023

MichaelMarcialis commented Feb 2, 2023

MichaelMarcialis left a comment

Choose a reason for hiding this comment

MichaelMarcialis Feb 2, 2023

Choose a reason for hiding this comment

MichaelMarcialis Feb 2, 2023

Choose a reason for hiding this comment

MichaelMarcialis Feb 2, 2023

Choose a reason for hiding this comment

MichaelMarcialis Feb 2, 2023

Choose a reason for hiding this comment

kibana-ci commented Feb 2, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

drewdaemon commented Feb 3, 2023

drewdaemon commented Jan 19, 2023 •

edited

Loading