Eui sharing top nav by nreese · Pull Request #21997 · elastic/kibana

nreese · 2018-08-15T14:14:55Z

EUI share top nav

nreese · 2018-08-15T14:18:11Z

@snide @cchaos Do you know why the tooltip is disabled when the radio group option is disabled? The tooltip behaves weirdly. When the menu is first opened, the tooltip is opened even though the user has not moused over the icon.

To reproduce the problem, checkout this PR and create a new dashboard. Before saving the dashboard, click the share top nav button. Select either option in the context menu. Notice how the tooltip is open when the panel first opens.

cchaos · 2018-08-15T14:29:52Z

My guess would be that it's trying to put focus into the popover and finding the first focusable element which would be the tooltip since the first radio is disabled?

nreese · 2018-08-15T14:39:41Z

sounds like a good theory. How do I make it stop doing that?

snide · 2018-08-15T14:58:51Z

That's definitely what it is. I tried looking at this local but it doesn't seem to work. This is using a context menu which by nature hijacks focus and places it in the first available item within the panel.

Off hand I can't think of a quick way around this other than reorganizing your form so that a disabled item isn't first (ex: moving short url before the other options).

snide · 2018-08-15T14:59:21Z

Happy to troubleshoot when I get started today (9AM PST)

cchaos · 2018-08-15T15:06:19Z

The short URL option can't be moved because it's dependent on the selection of choosing "snapshot". It's not available for saved objects.

Could adding autofocus on the first enabled radio work?

nreese · 2018-08-15T15:25:09Z

@cchaos that worked. I will have to create a PR in EUI to pass autoFocus down from the radio group option to the radio input

nreese · 2018-08-15T15:32:17Z

@snide @cchaos Another work around is to just change the order of options so that the disabled option is never first.

Which is the preferred solution? Changing the radio group order or passing autoFocus in?

cchaos · 2018-08-15T15:46:50Z

The nice think about swapping them is that when the saved object option is disabled the help text is more closely coupled with the disabled option. However, when the saved object option is enabled, it's set to the default picked option, which seems odd to me that the default wouldn't be first.

cchaos · 2018-08-15T15:51:29Z

I guess, just to make your life easier, swap the options :). But can you make an issue in EUI that we need to be able pass more options per radio from radio group?

elasticmachine · 2018-08-21T15:42:28Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-08-21T18:32:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…aringTopNav

elasticmachine · 2018-08-22T16:46:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

stacey-gammon · 2018-08-22T18:23:29Z

can trigger a console error:

Warning: Failed prop type: The prop textToCopyis marked as required inEuiCopy, but its value is undefined.

Steps to repro:

Open permalink share menu.
Switch to saved object type
Modify url and click enter

I think this is because url is stored on state, so when it's set to null after a refresh, it causes the object to re-render.

I made some code suggestions here: c61b3ed

They are:

Using enum type instead of string type for exportUrlAs
Combined getUrl and setUrl. Rather than read from passed in parameters, it's just reading the values off state.
Combined url and shortUrl. Made getUrl async so there is no need for the short url creation to communicate to the caller via the special this.state.shortUrl value, but through the return value of createShortUrl.
Changed type of this.state.url to not allow undefined, instead setting default value to an empty string to avoid the prop warning from above. Also got rid of resetUrl from setting the url to undefined. It turns off short url, so the function is then synchronous, so it should always set the url to something in setState. I don't think we have to clear it out before updating the url.

Take what you like!

Do you think it would be difficult to add a jest test to ensure saved object url copied does not include _a parameter? I made a mistake in my first suggestions code that failed to use the right url and jest tests still passed.

stacey-gammon · 2018-08-22T14:13:39Z

src/ui/public/share/components/share_context_menu.tsx

+  allowEmbed: boolean;
+  objectId?: string;
+  objectType: string;
+  getUnhashableStates: () => any[];


Think you can make this object[] to be a little more specific, until we have state definitions.

stacey-gammon · 2018-08-22T15:24:30Z

src/ui/public/share/components/share_url_content.tsx

+      ? `Can't share as saved object until the ${this.props.objectType} has been saved.`
+      : undefined;
+    return (
+      <EuiFormRow label="Generate the link as" helpText={generateLinkAsHelp}>


Maybe something to file in EUI, but this shows up as a clickable link which is a little strange.

elastic/eui#1136

stacey-gammon · 2018-08-22T15:42:55Z

src/ui/public/share/components/share_url_content.tsx

+    );
+  };
+
+  private handleShortUrlChange = async (evt: any) => {


Can you add a comment here to switch this to ChangeEvent<HTMLInputElement> or EuiSwitchChangeCallback once elastic/eui#1134 is fixed?

stacey-gammon · 2018-08-22T15:47:08Z

src/ui/public/share/components/share_url_content.tsx

+      try {
+        await this.createShortUrl();
+      } catch (fetchError) {
+        this.setState(


I don't think this will ever be hit because there is already a try/catch inside this.ceateShortUrl. I think you can get rid of this outer one and just add useShortUrl: false inside the catch from createShortUrl

The error was getting re-thrown. I have cleaned up the logic by moving creatingShortUrl into handleShortUrlChange so its easier to follow the logic

stacey-gammon · 2018-08-22T16:26:00Z

src/ui/public/share/components/share_url_content.tsx

+    const defaultUseShortUrl = false;
+
+    this.state = {
+      exportUrlAs: defaultExportUrlAs,


Can do:

enum ExportUrlAsType { EXPORT_URL_AS_SAVED_OBJECT = 'savedObject', EXPORT_URL_AS_SNAPSHOT = 'snapshot', } interface State { exportUrlAs: ExportUrlAsType; useShortUrl: boolean; isCreatingShortUrl: boolean; url?: string; shortUrl?: string; shortUrlErrorMsg?: string; } ... const defaultExportUrlAs = ExportUrlAsType.EXPORT_URL_AS_SNAPSHOT; .... id: ExportUrlAsType.EXPORT_URL_AS_SNAPSHOT, etc, etc

nreese · 2018-08-22T19:48:51Z

Combined url and shortUrl. Made getUrl async so there is no need for the short url creation to communicate to the caller via the special this.state.shortUrl value, but through the return value of createShortUrl.

The existing angular implementation stored shortUrl to avoid the network overhead of creating new shortUrl every time the checkbox is selected. The network overhead does not matter much, but if the implementation of /api/shorten_url ever changed you could get into a situation where each time the user checked the box then a new short url token would be created. This is not the case now but I think caching the results of /api/shorten_url prevent unneeded server calls.

stacey-gammon · 2018-08-22T20:03:59Z

Interesting. I don't think that would need to be part of state, if we wanted to cache the results. Could just store mapping of full url to short url, look it up if available and use that instead of createShortUrl.

something like:

       const snapshotUrl = this.getSnapshotUrl();
       if (this.state.useShortUrl) {
          if (!this.shortUrlCache.contains(snapshotUrl) {
            this.shortUrlCache[snapshotUrl] = await this.createShortUrl();
          }
          url = this.shortUrlCache[snapshotUrl];
        } else {
          url = snapshotUrl;
        }
}

…py, but its value isundefined

elasticmachine · 2018-08-22T22:28:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…aringTopNav

elasticmachine · 2018-08-28T13:49:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

stacey-gammon

Did not pull latest code. Extra test looks good!

elasticmachine · 2018-08-28T15:41:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

nreese · 2018-08-28T15:51:30Z

jenkins, test this

elasticmachine · 2018-08-28T17:53:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* just getting the popover to open and start laying out the context menu * pass getUnhashableStates to ShareMenu * generate original and snapshot ids * move state into ShareUrlContent * start working on form * use radio group * add input for creating short URL * display URL in alert until copy functionallity gets migrated to EUI * allowEmbed prop * replace share directive with showShareContextMenu * fix button styling * add jest test for share_context_menu * use EuiCopy to copy URL, add jest test for ShareUrlContent component * clean up * display short URL create error message in form instead of with toast * switch option order so disbaled option can not be first * fix discover share functional tests * add functions required by reporting * typescript * remove empty file * fix typescript compile error * move import so jest tests work * fix Failed prop type: The proptextToCopyis marked as required inEuiCopy, but its value isundefined * move shortUrl out of react state and into Component object * getUnhashableStates type from any[] to object[] * add comment about type change once EUI issue is solved * add functional test for saved object URL sharing * remove commit

nreese added 14 commits August 13, 2018 15:33

just getting the popover to open and start laying out the context menu

7878c0c

pass getUnhashableStates to ShareMenu

b2660fe

generate original and snapshot ids

8a84489

move state into ShareUrlContent

8216b6e

start working on form

d871e10

use radio group

adacc96

add input for creating short URL

d8c7dc7

display URL in alert until copy functionallity gets migrated to EUI

f30edf3

allowEmbed prop

fccaa2a

replace share directive with showShareContextMenu

82650e7

fix button styling

b480e99

add jest test for share_context_menu

113f036

merge with master

61775d7

use EuiCopy to copy URL, add jest test for ShareUrlContent component

89f590b

nreese added :Sharing v7.0.0 v6.5.0 labels Aug 15, 2018

clean up

23fe41d

display short URL create error message in form instead of with toast

d98c2fa

switch option order so disbaled option can not be first

8cf0a6e

remove empty file

d2725e6

fix typescript compile error

d53bf1f

nreese added 2 commits August 22, 2018 08:38

Merge branch 'master' of https://github.com/elastic/kibana into euiSh…

97dfe0a

…aringTopNav

move import so jest tests work

b7d9553

stacey-gammon reviewed Aug 22, 2018

View reviewed changes

nreese added 4 commits August 22, 2018 14:16

fix Failed prop type: The proptextToCopyis marked as required inEuiCo…

aa4f0e2

…py, but its value isundefined

move shortUrl out of react state and into Component object

214a4e0

getUnhashableStates type from any[] to object[]

6c825bb

add comment about type change once EUI issue is solved

187e296

nreese mentioned this pull request Aug 24, 2018

[Feature Request] Auto copy after generating Short URL #18872

Closed

nreese added 3 commits August 28, 2018 06:02

Merge branch 'master' of https://github.com/elastic/kibana into euiSh…

c3985f7

…aringTopNav

add functional test for saved object URL sharing

7ef83c4

remove commit

0f5e391

stacey-gammon approved these changes Aug 28, 2018

View reviewed changes

nreese merged commit 9d24c00 into elastic:master Aug 28, 2018

nreese mentioned this pull request Aug 28, 2018

[6.x] Eui sharing top nav (#21997) #22468

Merged

Conversation

nreese commented Aug 15, 2018

Uh oh!

nreese commented Aug 15, 2018

Uh oh!

cchaos commented Aug 15, 2018

Uh oh!

nreese commented Aug 15, 2018

Uh oh!

snide commented Aug 15, 2018

Uh oh!

snide commented Aug 15, 2018

Uh oh!

cchaos commented Aug 15, 2018

Uh oh!

nreese commented Aug 15, 2018

Uh oh!

nreese commented Aug 15, 2018

Uh oh!

cchaos commented Aug 15, 2018

Uh oh!

cchaos commented Aug 15, 2018

Uh oh!

elasticmachine commented Aug 21, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Aug 21, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Aug 22, 2018

💚 Build Succeeded

Uh oh!

stacey-gammon commented Aug 22, 2018

Uh oh!

stacey-gammon Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

stacey-gammon Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

nreese Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

stacey-gammon Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

stacey-gammon Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

nreese Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

stacey-gammon Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

nreese commented Aug 22, 2018

Uh oh!

stacey-gammon commented Aug 22, 2018

Uh oh!

elasticmachine commented Aug 22, 2018

💚 Build Succeeded

Uh oh!

elasticmachine commented Aug 28, 2018

💚 Build Succeeded

Uh oh!

stacey-gammon left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 28, 2018

💔 Build Failed

Uh oh!

nreese commented Aug 28, 2018

Uh oh!

elasticmachine commented Aug 28, 2018

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects