[Lens] Don't block render on missing field #149262
Conversation
…ck-render-on-missing-field
…m:andrewctate/kibana into 143673/dont-block-render-on-missing-field
|
@drewdaemon: Thanks for those comments. Some quick responses below:
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.
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. |
Done. This is what it looks like
WDYT? |
|
@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. |
|
A couple of comments @MichaelMarcialis and @drewdaemon :
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. |
|
@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. |
@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. |
There was a problem hiding this comment.
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.
| <EuiIcon | ||
| {...baseIconProps} | ||
| type="color" | ||
| color="text" |
There was a problem hiding this comment.
Can we change this to subdued to match the other prepend icons?
| color="text" | |
| color="subdued" |
| <EuiButtonIcon | ||
| className="lnsLayerPanel__dimensionRemove" | ||
| data-test-subj="indexPattern-dimension-remove" | ||
| iconType="cross" | ||
| iconSize="s" | ||
| iconType="trash" | ||
| size="s" |
There was a problem hiding this comment.
Possible to reduce the size here to xs to fit more comfortably in the dimension item box?
| size="s" | |
| size="xs" |
| > | ||
| <ColorIndicator accessorConfig={accessorConfig}>{children}</ColorIndicator> | ||
| </EuiLink> | ||
| <EuiFlexItem> |
There was a problem hiding this comment.
Is EuiFlexItem needed here? May be able to replace with a regular old div.
| {errorCount > 0 && ( | ||
| <> | ||
| <EuiIcon type={IconError} /> | ||
| {errorCount} | ||
| </> | ||
| )} | ||
| {warningCount > 0 && ( | ||
| <> | ||
| <EuiIcon | ||
| type={IconWarning} | ||
| css={css` | ||
| margin-left: 4px; | ||
| `} | ||
| /> | ||
| {warningCount} | ||
| </> | ||
| )} |
There was a problem hiding this comment.
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?
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
kibanamachine
added
v8.7.0
backport:skip
|
@MichaelMarcialis I've merged the PR with the understanding that I will address your final round of feedback in #150241 |
Summary
Resolve #143673
Resolve #147485
This PR introduces the new error/warning UI outlined in #147485. In addition
Screenshots
Editor
Errors
Warnings
Both
Embeddable
Errors
Warnings
Both
Checklist
Delete any items that are not applicable to this PR.