[Lens] Thresholds: Add text to markers body #113629

dej611 · 2021-10-01T14:25:25Z

Summary

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
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
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)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

…reshold_panel.tsx Co-authored-by: Michael Marcialis <michael@marcial.is>

ghudgins · 2021-10-01T16:26:00Z

can we align this in the center

dej611 · 2021-10-04T11:01:32Z

@elasticmachine merge upstream

kibanamachine · 2021-10-04T11:01:34Z

merge conflict between base and head

MichaelMarcialis · 2021-10-05T20:54:20Z

In terms of where to put the Show marker label switch.. I have no better idea. Maybe instead of being about "showing label", something about what to show in the marker may work best? Something like Marker: None | Icon | Label | Full?

That is an interesting idea to unify the icon and text labels under a common term. The two concerns I have with such an approach are 1) that it complicates the icon selection flow (requires additional clicks, must conditionally disable the icon selector, etc.) and 2) that it turns a simple switch into a more complex selector or button group. I'm curious if we can instead address @mbondyra's concerns with some verbiage and ordering changes.

For example:

Rename Show label to Show display name
Move the Show display name row to appear immediately after Display name
Move the Icon row to appear immediately after Show display name
Rename Marker position to Decoration position
Move the Decoration position row to appear immediately after Icon

So the new final order would be something like:

Display name
Show display name
Icon
Decoration position
Color
Line style
Line thickness
Fill

Would an approach like this address the original concern?

dej611 · 2021-10-06T13:53:56Z

I've updated the panel with the new order:

Also aligned the content in the tooltip to mention display name rather than label:

The switch label is a bit too long for smaller screens:

MichaelMarcialis

This is looking great, @dej611! Leaving you a few comments and questions below for my review:

Do you have any thoughts about changing all the references to the Display name input to simply Name? Display name always seemed overly verbose to me, and doing so means we could also shorten Show display name to Show name. CCing @ghudgins for thoughts on this as well.
Regarding @markov00's excellent suggestions for improving the in-visualization threshold label styling, is this an enhancement the dataviz team would provide via an enhancement to Elastic charts? Or is the assumption that such an enhancement would be on us (the Lens team) to implement on our end? If it's something we'd be responsible for implementing, is this something we wish to address in this PR or separately in a future issue?

MichaelMarcialis · 2021-10-06T19:50:20Z

x-pack/plugins/lens/public/xy_visualization/expression_thresholds.tsx

+    <div
+      style={{
+        display: 'inline-block',
+        overflow: 'hidden',
+        width: THRESHOLD_MARKER_SIZE,
+        lineHeight: 1.5,
+      }}
+    >
+      <div
+        className="eui-textTruncate"
+        style={{
+          display: 'inline-block',
+          whiteSpace: 'nowrap',
+          transform: 'translate(0, 100%) rotate(-90deg)',
+          transformOrigin: '0 0',
+          maxWidth: THRESHOLD_MARKER_SIZE * 3,
+        }}
+      >
+        {label}
+        <div
+          style={{
+            float: 'left',
+            marginTop: '100%',
+          }}
+        />
+      </div>
+    </div>


Aside from the styles that reference the THRESHOLD_MARKER_SIZE variable, what is the reason for using inline styles versus using classes that apply styles via an external SCSS file? I'm only asking for the sake of consistency with how we appear to handle styles elsewhere in Lens.

MichaelMarcialis · 2021-10-06T19:53:07Z

x-pack/plugins/lens/public/xy_visualization/expression_thresholds.tsx

+        <div
+          style={{
+            float: 'left',
+            marginTop: '100%',
+          }}
+        />


If you end up changing these inline styles to SCSS (as described in the previous comment), consider also switching from styling this empty div element to an ::after pseudo element.

flash1293 · 2021-10-07T03:45:24Z

If it's something we'd be responsible for implementing, is this something we wish to address in this PR or separately in a future issue?

To me it makes sense what @ghudgins wrote here: #112921 (comment)

if we are going to offer the experience we have right now anyway, let’s start with that and follow up with additional configuration options. We can discuss offline though.

dej611 · 2021-10-07T12:35:02Z

@elasticmachine merge upstream

dej611 · 2021-10-07T15:27:47Z

@elasticmachine merge upstream

dej611 · 2021-10-07T16:12:21Z

@elasticmachine merge upstream

MichaelMarcialis

Thanks for making those changes, @dej611. Left one small suggestion, but otherwise looks good to me. Approving now so I don't hold you up.

MichaelMarcialis · 2021-10-07T16:59:50Z

x-pack/plugins/lens/public/xy_visualization/expression_thresholds.scss

+.lnsXyDecorationRotatedWrapper {
+  display: inline-block;
+  overflow: hidden;
+  line-height: 1.5;


Suggest using the $euiLineHeight variable here:

Suggested change

line-height: 1.5;

line-height: $euiLineHeight;

stratoula

Code LGTM, I tested it locally and it seems to work fine :) I think for a first iteration is a very cool addition!

stratoula · 2021-10-08T08:01:12Z

x-pack/plugins/lens/common/expressions/xy_chart/axis_config.ts

@@ -187,6 +188,10 @@ export const yAxisConfig: ExpressionFunctionDefinition<
      options: ['auto', 'above', 'below', 'left', 'right'],
      help: 'The placement of the icon for the threshold line',
    },
+    textVisibility: {
+      types: ['boolean'],
+      help: 'Visibility of the marker label',


super nit: Can we also add to the help text that it is used for the reference line? Otherwise, both the variable name and help text are very generic. Moreover I can't see the word marker anywhere in the UI so this confuses even more

dej611 · 2021-10-11T08:25:38Z

@elasticmachine merge upstream

kibanamachine · 2021-10-11T10:29:28Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	825	834	+9

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`	234	235	+1

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.0MB	1.0MB	+3.7KB

Page load bundle

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

id	before	after	diff
`lens`	39.1KB	39.2KB	+88.0B

Unknown metric groups

API count

id	before	after	diff
`lens`	252	253	+1

History

💔 Build #159027 failed ecae4cd
💔 Build #159001 failed ae94760
💔 Build #158909 failed 2d285fc
💔 Build #158876 failed 8a16fc8
💚 Build #158470 succeeded 3bc9b3b

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

* 🐛 Add padding to the tick label to fit threshold markers * 🐛 Better icon detection * 🐛 Fix edge cases with no title or labels * 📸 Update snapshots * ✨ Add icon placement flag * ✨ Sync padding computation with marker positioning * 👌 Make disabled when no icon is selected * ✨ First text on marker implementation * 🐛 Fix some edge cases with auto positioning * Update x-pack/plugins/lens/public/xy_visualization/xy_config_panel/threshold_panel.tsx Co-authored-by: Michael Marcialis <michael@marcial.is> * 🐛 Fix minor details * 💄 Small tweak * ✨ Reduce the padding if no icon is shown on the axis * ✅ Fix broken unit tests * 💄 Fix vertical text centering * 🚨 Fix linting issue * 🐛 Fix issue * 💄 Reorder panel inputs * 💄 Move styling to sass * 👌 Address feedback Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Michael Marcialis <michael@marcial.is>

kibanamachine · 2021-10-11T10:35:11Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

kibanamachine · 2021-10-13T10:35:00Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

* 🐛 Add padding to the tick label to fit threshold markers * 🐛 Better icon detection * 🐛 Fix edge cases with no title or labels * 📸 Update snapshots * ✨ Add icon placement flag * ✨ Sync padding computation with marker positioning * 👌 Make disabled when no icon is selected * ✨ First text on marker implementation * 🐛 Fix some edge cases with auto positioning * Update x-pack/plugins/lens/public/xy_visualization/xy_config_panel/threshold_panel.tsx Co-authored-by: Michael Marcialis <michael@marcial.is> * 🐛 Fix minor details * 💄 Small tweak * ✨ Reduce the padding if no icon is shown on the axis * ✅ Fix broken unit tests * 💄 Fix vertical text centering * 🚨 Fix linting issue * 🐛 Fix issue * 💄 Reorder panel inputs * 💄 Move styling to sass * 👌 Address feedback Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Michael Marcialis <michael@marcial.is> Co-authored-by: Marco Liberati <dej611@users.noreply.github.com> Co-authored-by: Michael Marcialis <michael@marcial.is>

dej611 and others added 27 commits September 23, 2021 12:09

🐛 Add padding to the tick label to fit threshold markers

d7f6363

🐛 Better icon detection

6f59341

🐛 Fix edge cases with no title or labels

a5c5c09

📸 Update snapshots

e705e0f

Merge branch 'master' into fix/112793

a61c78b

Merge branch 'master' into fix/112793

33845bb

✨ Add icon placement flag

5a5270a

✨ Sync padding computation with marker positioning

6cfad94

Merge branch 'master' into fix/112793

6485fa7

👌 Make disabled when no icon is selected

af96b0a

Merge branch 'master' into fix/112793

5a9ee3b

✨ First text on marker implementation

8eedc11

🐛 Fix some edge cases with auto positioning

44a70a4

Merge branch 'fix/112793' of github.com:dej611/kibana into fix/112793

fbb372c

Merge branch 'fix/112793' into fix/112921

21c60b7

Update x-pack/plugins/lens/public/xy_visualization/xy_config_panel/th…

5b4374c

…reshold_panel.tsx Co-authored-by: Michael Marcialis <michael@marcial.is>

🐛 Fix minor details

20cecf4

Merge branch 'fix/112793' into fix/112921

c5b4e73

💄 Small tweak

f8bb856

Merge remote-tracking branch 'upstream/master' into fix/112793

82a83aa

Merge branch 'fix/112793' into fix/112921

43a3a7f

Merge remote-tracking branch 'upstream/master' into fix/112793

ac46286

Merge branch 'fix/112793' into fix/112921

849e25c

Merge remote-tracking branch 'upstream/master' into fix/112793

11e2372

Merge branch 'fix/112793' into fix/112921

3fd11f5

✨ Reduce the padding if no icon is shown on the axis

32af982

✅ Fix broken unit tests

7188aa7

💄 Reorder panel inputs

3bc9b3b

MichaelMarcialis reviewed Oct 6, 2021

View reviewed changes

kibanamachine and others added 2 commits October 7, 2021 08:35

Merge branch 'master' into fix/112921

8a16fc8

💄 Move styling to sass

2d285fc

dej611 requested a review from a team as a code owner October 7, 2021 13:27

Merge branch 'master' into fix/112921

ae94760

dej611 requested a review from MichaelMarcialis October 7, 2021 15:27

Merge branch 'master' into fix/112921

ecae4cd

MichaelMarcialis approved these changes Oct 7, 2021

View reviewed changes

stratoula approved these changes Oct 8, 2021

View reviewed changes

stratoula reviewed Oct 8, 2021

View reviewed changes

👌 Address feedback

38e7c7b

Merge branch 'master' into fix/112921

62b15d1

dej611 merged commit 1bf09e6 into elastic:master Oct 11, 2021

kibanamachine mentioned this pull request Oct 11, 2021

[7.x] [Lens] Thresholds: Add text to markers body (#113629) #114460

Merged

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 13, 2021

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Thresholds: Add text to markers body #113629

[Lens] Thresholds: Add text to markers body #113629

dej611 commented Oct 1, 2021 •

edited

Loading

ghudgins commented Oct 1, 2021

dej611 commented Oct 4, 2021

kibanamachine commented Oct 4, 2021

MichaelMarcialis commented Oct 5, 2021

dej611 commented Oct 6, 2021

MichaelMarcialis left a comment

MichaelMarcialis Oct 6, 2021

MichaelMarcialis Oct 6, 2021

flash1293 commented Oct 7, 2021

dej611 commented Oct 7, 2021

dej611 commented Oct 7, 2021

dej611 commented Oct 7, 2021

MichaelMarcialis left a comment

MichaelMarcialis Oct 7, 2021

stratoula left a comment

stratoula Oct 8, 2021 •

edited

Loading

dej611 commented Oct 11, 2021

kibanamachine commented Oct 11, 2021

API count

kibanamachine commented Oct 11, 2021

kibanamachine commented Oct 13, 2021

[Lens] Thresholds: Add text to markers body #113629

[Lens] Thresholds: Add text to markers body #113629

Conversation

dej611 commented Oct 1, 2021 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

ghudgins commented Oct 1, 2021

dej611 commented Oct 4, 2021

kibanamachine commented Oct 4, 2021

MichaelMarcialis commented Oct 5, 2021

dej611 commented Oct 6, 2021

MichaelMarcialis left a comment

Choose a reason for hiding this comment

MichaelMarcialis Oct 6, 2021

Choose a reason for hiding this comment

MichaelMarcialis Oct 6, 2021

Choose a reason for hiding this comment

flash1293 commented Oct 7, 2021

dej611 commented Oct 7, 2021

dej611 commented Oct 7, 2021

dej611 commented Oct 7, 2021

MichaelMarcialis left a comment

Choose a reason for hiding this comment

MichaelMarcialis Oct 7, 2021

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

stratoula Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

dej611 commented Oct 11, 2021

kibanamachine commented Oct 11, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

kibanamachine commented Oct 11, 2021

💚 Backport successful

kibanamachine commented Oct 13, 2021

dej611 commented Oct 1, 2021 •

edited

Loading

stratoula Oct 8, 2021 •

edited

Loading