Redesign Share UI to emphasize difference between Original URLs and Snapshot URLs.

[cjcenizal]

This supersedes #8157.

Redesign Share UI to emphasize difference between Original URLs and Snapshot URLs.

• Addresses Improve clarity around what is being shared in share UI #8051
• Blocked by [state] store actual state value in session storage #8022

Discover

Unsaved

Saved

Short URL

Dashboard

Unsaved

Saved

Short URL

Visualization

Unsaved

Saved

Short URL

[cjcenizal]

I got some design feedback from @alt74 and we updated the UI to look like this:

[spalger]

For the record, I thought the yellow warning text was 💯 . It's seems pretty easy to miss completely now.

[spalger]

I think this should be more about compatibility with consumers of the URL, rather than sharing it. Maybe something like...

We recommend sharing shortened snapshot urls for maximum compatibility. Internet Explorer has URL length restrictions, and some wiki and markup parsers don't do well with the full-length version of the snapshot URL, but the short URL should work great.

[spalger]

If you get a short url, and then the url changes, the short url persists and points to the wrong long-url.

[spalger]

How would you feel about putting all the urls under this.urls = { original, snapshot, ... }. Then, in updateUrls() you can just overwrite the this.urls object in order to deal with #8172 (comment)

[spalger]

Some notes, only one bug, looking good

[cjcenizal]

Re the warning, I think it's better to present it subdued, since we don't really want to draw the user's attention to it if they're looking elsewhere. If the user is trying to share the Original, they'll be looking in that area already so I think the chances are good that they'll read the warning, despite the subdued presentation.

[spalger]

I think this is a better user experience, but I'm worried that it could create a bunch of never-used short urls (which in turn are elasticsearch docs)

[spalger]

I think this is a step in the right direction in most every way, but the user should probably be able to toggle the short flag, rather than it being a one-way click. Right now, the only way to disable short URL creation is to close and re-open the share ui.

[Bargs]

Coming to this with semi-fresh eyes, the term Original URL still seems confusing to me. I had to re-read the descriptive text a few times to remember everything we talked about, and users won't have that context. I've been mulling it over and I have a few questions:

• Is it useful for the snapshot URLs to contain a link to the vis they were loaded from? A snapshot is a bundle of state with no real relation to the original saved vis. Perhaps we should fully embrace that fact and remove the vis ID from the URL. That way all snapshots are created equal, whether they originated from a saved vis or not. This makes the behavior more predictable for users, snapshots are always a purely transient entity that allow you to share the state of a vis/dashboard as you currently see it, without saving. It would also allow us to remove this confusing line of text, which makes no sense if the Vis was never saved: "Edits to the saved visualization won't be visible via this URL."
• If you buy into the above what would you think about changing "Share Original" to "Share Saved Viz"/"Share Saved Dashboard" or maybe just "Share Link"? If we do the first bullet, I think that gives us an opportunity to better separate the two sharing mechanisms. One mechanism shares links to named things that are saved (and this becomes the primary way things are shared). The other mechanism shares transient snapshots (this becomes more of a special use case for ad hoc reports).
• We can check the dirty state of the app to see if changes have been made since the last save (like in reporting). Maybe we should dynamically display a warning on the Original URL side if there are unsaved changes.
• Building on all of the above, maybe we should only display one type of sharing at a time, instead of placing them side-by-side? For an unsaved vis, you would only see the snapshot option which would include a message about saving the vis if you want to share future changes. If the vis was saved at some point in the past, we show the "original url" view by default. If we detect unsaved changes, we display a warning message that says you should save your changes or if you'd like to share a snapshot, "click here" and it'll switch to the snapshot view. I think we could even provide the short url by default in this scenario since it's the recommended URL and it would only get generated when a user explicitly says they want a snapshot.

Sorry if this is a giant rambling mess, I've been thinking though it as I type. I guess the overall thing I'm trying to get at is, I think we can create a better separation between the two sharing mechanisms and then guide the user to the correct one based on the current context. Rather than display everything all the time and force the user to read and understand a bunch of stuff right off the bat, we can guide them to the correct decision by hiding inappropriate options and providing helpful, dynamic messages.

[cjcenizal]

Removing reference to original visualization from Snapshot URLs

I think your argument makes sense and I think if we want to preserve a reference to the original, we should do that by persisting that reference in a query param, as opposed to part of the URL. This will make it clear that it's simply a reference, and not actually a contributing factor to the way the visualization is rendered. I think we should do this in a subsequent PR though.

Share Saved vs Share Original

I'm down with "Share Saved"! Good idea!

Unsaved warning message

Another good idea. I think this is best tackled in another PR.

Showing different UIs based on different states

I think this is worth exploration, but it's also not a blocker for this PR. Are you OK with merging this after I make the change to the "Original" naming pattern, and then we can explore next steps for improving it?

[cjcenizal]

Updated UI per @Bargs suggestion:

[epixa]

How about simply $scope.allowEmbed !== 'false' here? It took me a moment to grok what this what was doing.

[spalger]

Or combine this and the if statement above into a switch()?

[cjcenizal]

I tried the switch idea, but couldn't figure out a case to match the $scope.allowEmbed !== undefined condition.

Re the original suggestion, I think equality comparisons are easier to think about than inequality, especially when comparing booleans. When you see an equality comparison in a ternary, you can associate the happy path with the values and variables in the comparison. But with an inequality comparison, you need to think of the opposite of the compared value to imagine the happy path, which I think is one extra unnecessary step.

Is the ternary the problem? Do you think this is easier to follow @epixa ?

this.allowEmbed = true;

// Default to allowing an embedded IFRAME, unless it's explicitly set to false.
if ($scope.allowEmbed === 'false') {
  this.allowEmbed = false;
}

I can also make the comment clearer if you have any suggestions.

[epixa]

I'm not sure if that makes it clearer or not, but I don't think it's worth splitting this into different statements. Honestly though, I don't feel strongly about it one way or another.

[spalger]

function castAllowEmbedToBoolean(allowEmbed) {
  switch (allowEmbed) {
    case 'true':
      return true;
    case undefined:
    case 'false':
      return false;
    default:
      throw new TypeError('...');
  }
}

this.allowEmbed = castAllowEmbedToBoolean($scope.allowEmbed);

[cjcenizal]

Thanks @spalger , I see what you're saying. I prefer the way it is currently because I think it's clearer that true is the default value unless "false" is passed in.

[epixa]

Minor nitpick here: but if you use angular's $document service instead of directly accessing the global document, it makes testing a good deal easier.

[cjcenizal]

Sounds good but how does it make it easier?

[epixa]

You can mock the service so it doesn't actually require dom access and instead verify that the appropriate selector is being passed.

[Bargs]

Embedded iframe isn't an option for Saved object sharing? I think this would be the preferred link to share in an iframe.

[Bargs]

We're mixing different title casing styles here, should it be "Share Saved Dashboard" or "Share snapshot"?

[cjcenizal]

Do we have copy-writing rules in place? Personally I prefer sentence-casing (capitalizing the first word) over title-casing (capitalizing nouns and verbs) in general, and using capitalized words to indicate special terms, like "Snapshot".

[Bargs]

I was sort of thinking of "Saved Dashboard" as a special term, in contrast to "Snapshot". Not sure about copy rules though. @debadair ?

[Bargs]

Other than the two things I mentioned above, functionality LGTM

[cjcenizal]

@Bargs I added the option to embed an iframe to a saved visualization, but I added on the global state to the URL, so that it won't just load the default time range. Let's fix the capitalization later, after @debadair chimes in.

[epixa]

If you rebase this, the build should pass.

[spalger]

This should probably be using bonafide url parsing and formatting

[cjcenizal]

How's this:

const parsedUrl = parseUrl(window.location.href);
return formatUrl({
  protocol: parsedUrl.protocol,
  auth: parsedUrl.auth,
  host: parsedUrl.host,
  path: parsedUrl.path,
  search: {
    _g: parsedUrl.search._g,
  },
});

I can definitely see how this is more consistent with the rest of the codebase. Are there other reasons to write it this way?

[spalger]

You actually need to parse the parent url and the url in the hash, beyond that I'm pretty sure that search needs to be query (search is for strings, query for objects, iirc).

As to why, adding a query string param to the non-hash part of the url would completely break this functionality:

https://localhost:5601/cdf/app/kibana?flag=true#/discover/New-Saved-Search?_g=()&_a=(columns:!(_source),filters:!(),index:'logstash-*',interval:auto,query:(query_string:(analyze_wildcard:!t,query:'*')),sort:!('@timestamp',desc))

produces the share url

https://localhost:5601/cdf/app/kibana?_g=()

When it comes to parsing standard formats though, I always urge people to use the proper encoding/decoding tools. While simply splitting on '?' works today, there is good reason to expect that it at some point an edge case will show up that may or may-not be very hard to debug.

[cjcenizal]

Oh, I see what you're saying! I forgot about that gotcha about the way Angular stores stuff after the hash. Great call, thanks.

[spalger]

LGTM

[epixa]

@cjcenizal Can you make sure the docs are updated with new screenshots and such? https://www.elastic.co/guide/en/kibana/current/dashboard.html#sharing-dashboards

That doesn't necessarily have to happen in this PR if you're still trying to get this feature in for 5.0, but it should happen in a follow up as soon as possible.

[epixa]

@cjcenizal I created backport PRs for this since we're running out of time to get stuff into 5.0 and I wanted the builds going.

[cjcenizal]

Thanks @epixa