Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Time to Visualize] Library Notification Popover #79581

Merged

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Oct 5, 2020

Summary

in the Time to VIsualize project, it's important to provide many obvious ways for the user to enter into the new flow. The library notification added in #76122 can act as a good entry point/call to action by prompting the user to unlink the panel from the library.

Oct-07-2020 12-55-49

TODO:

  • finalize design of the popover - @ryankeairns, do you have any feedback or ideas for this popover?
  • finalize copy. It currently shows the same copy as the tooltip did before. Maybe an explanation of what the unlink button does may help? Would a title in the popover be good?

How to test this?

This is just one part of a much larger change, so all changes in this PR are hidden behind a configuration value.

  1. you'll need to set the allowByValueEmbeddables config value to true:

    allowByValueEmbeddables: schema.boolean({ defaultValue: false }),

  2. Open a dashboard, and use the add panels functionality to add an embeddable from the library. You should see the new notification show up on the right side.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@ryankeairns
Copy link
Contributor

Hey, yeah, I’ll give it some thought.

One initial reaction is - how do we indicate the library status within the editing application? That feels like the point in the flow with the greatest chance for 'breaking' things.

My guess is that people will continue on with the ‘Edit this panel’ action from the Dashboard 'gear' menu at which point there is increased potential of updating a library item inadvertently. We've kept that 'Save and return' flow purposefully streamlined (single click), so how can we indicate the potential for broader impact without adding another hurdle (i.e. modal), for example. Perhaps it's something as simple as an icon alongside the Save and return? I'll give this more thought as well.

@ThomThomson
Copy link
Contributor Author

This is certainly another important consideration. Right now the only difference is the wording on the 'save as' button.

By Value
Screen Shot 2020-10-06 at 9 55 04 AM

By Reference
Screen Shot 2020-10-06 at 9 54 38 AM

A subtle indicator with a warning on hover could certainly help differentiate the two modes of the editor.

A more drastic approach could be to add a button that is the opposite of the 'save to library' button. The 'save to library' button acts as an escape hatch away from by value, but maybe we also need an escape hatch button into by value. The only problem I can think of is the copy for that button... 'save to dashboard' doesn't really work... 'embed' sounds like it's doing something else...

@ryankeairns
Copy link
Contributor

Ah right, I hadn't thought that all the way through. This feels like it could slip into something messier than I'd initially imagined. The existing buttons (Save to library and Save as) seem sufficient for now.

Perhaps we could do some sort of in-page indicator similar to the badge on the panel. Hold on that for now and I'll do some more tinkering.

@ryankeairns
Copy link
Contributor

To the point, I like the popover and have only a couple of minor feedback items:

  1. Let's have @gchaps take a look at the wording. One suggestion is to just have the button label read 'Unlink from library' (drop the item)
  2. I poked around other apps and found a couple of similar popover use cases. In short, aligning the button to the right and using the primary fill would match these other cases:

Screen Shot 2020-10-06 at 3 09 12 PM

Screen Shot 2020-10-06 at 3 14 30 PM


In addition (beyond this PR), we should consider similar changes elsewhere. For example, we could likewise indicate when an item is in the library from within an editing app (e.g. Viz/Lens). We'd have to think through the conditions of when to display this badge, but having a similar UX at this level would likely be helpful as its closer to the point of potentially affecting multiple dashboards.

Screen Shot 2020-10-06 at 2 07 14 PM

@ryankeairns
Copy link
Contributor

Forgot one other thing... a toast confirmation would be helpful to confirm once an item has been added to/removed from the library.

@ThomThomson
Copy link
Contributor Author

ThomThomson commented Oct 7, 2020

Thanks for the design input!

I've added a couple toasts to the 'add to library' and 'unlink from library' actions:

Screen Shot 2020-10-07 at 1 12 12 PM

Screen Shot 2020-10-07 at 1 09 01 PM

I have also reworked the design of the popover to be more in line with other similar popovers, and changed the library notification icon from a badge to an icon button to make it even more inconspicuous - as well as allowing me to add a title to the popover.

Oct-07-2020 12-55-49

@ThomThomson ThomThomson force-pushed the feature/libraryNotificationExecute branch from 8b3f7b1 to a626fac Compare October 7, 2020 17:32
@ryankeairns
Copy link
Contributor

@ThomThomson thanks for making these changes. I just gave it a run through and it both looks and works well!

@ThomThomson ThomThomson force-pushed the feature/libraryNotificationExecute branch from 0e82578 to ebc9a9d Compare October 7, 2020 18:25
@ThomThomson ThomThomson marked this pull request as ready for review October 7, 2020 20:22
@ThomThomson ThomThomson requested a review from a team October 7, 2020 20:22
@ThomThomson ThomThomson requested a review from a team as a code owner October 7, 2020 20:22
@ThomThomson ThomThomson added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Oct 7, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas (Team:Canvas)

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a hard time running this, so can't verify if it works. Code changes look good.
One question though - can we return the previous "Libary" badge look?

@ryankeairns
Copy link
Contributor

ryankeairns commented Oct 8, 2020

I'm having a hard time running this, so can't verify if it works. Code changes look good.
One question though - can we return the previous "Libary" badge look?

While the previous badge looked ok on its own, we're having to consider it in the context of several other new/upcoming panel elements. For that reason, we've opted to go with this more minimal approach and will look at a larger redesign of the edit state as described here: #79916

@ThomThomson
Copy link
Contributor Author

ThomThomson commented Oct 8, 2020

@majagrubic, you're right that the previous design was nicer looking. I changed this to an EuiIconButton for three reasons:

  1. There may be a lot of them on a dashboard, so it's important that it's super subtle.
  2. It's now a clickable button that opens a popover which gives plenty of other opportunities to show the required text, and it would be redundant to have the 'library' text show up both in the popover, and on the button itself.
  3. We are eventually going to move in a direction where many of the actions on the top bar are icons only. See [Dashboard] Improve the panel layout in edit mode #79916 for some preliminary designs.

Oh, @ryankeairns beat me to it!

@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, didn't test

@ThomThomson ThomThomson force-pushed the feature/libraryNotificationExecute branch from 5520c64 to a9a92a9 Compare October 13, 2020 17:24
@majagrubic
Copy link
Contributor

Managed to get it running 🎉 Tested in Chrome 85 on Mac OSX. Looks great. Unlinking / linking works as expected.
One minor thing I notice, which is probably unrelated to this PR, but putting it here so it doesn't get lost:

  1. Add a panel by value
  2. Add it to a library - give it title
  3. Choose "Hide title"
  4. Unlink from library -> after unlinking the title is shown (whereas I would expect it to stay hidden)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
dashboard 168 169 +1

page load bundle size

id before after diff
dashboard 340.0KB 344.4KB +4.4KB
embeddable 301.4KB 301.5KB +76.0B
total +4.5KB

History

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

@ThomThomson ThomThomson merged commit 2656ffc into elastic:master Oct 14, 2020
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Oct 14, 2020
* Added popover to the library notification which acts as an unlink shortcut
ThomThomson added a commit that referenced this pull request Oct 14, 2020
* Added popover to the library notification which acts as an unlink shortcut
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame Project:TimeToVisualize release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants