add an option for url types so that it can be opened in current tab (master branch) #13209

fbaligand · 2017-07-30T15:30:59Z

In Management tab > Index Patterns, in field edition form with "URL" format and "Link" type, this PR adds a checkbox to indicate that this link is to be open in current tab (default is "in another tab", ie. _blank).

This is particularly useful when your link points to a kibana dashboard with filtered params.

Fix #12668

elasticmachine · 2017-07-30T15:31:01Z

Can one of the admins verify this patch?

fbaligand · 2017-08-05T05:02:45Z

@epixa
Could you review this PR ?

epixa · 2017-08-09T14:45:57Z

@fbaligand Sorry for the lack of response, I was on vacation last week and am still catching up on notifications. I'm sure we can get a review on this soon.

epixa · 2017-08-09T14:51:55Z

jenkins, test this

chrisronline

Thanks for the contribution! It looks awesome! Just one minor issue and it'll be good to go!

chrisronline · 2017-08-09T15:09:52Z

src/ui/public/field_format_editor/editors/url/url.html

@@ -7,6 +7,13 @@
  </select>
 </div>

+<div class="checkbox" ng-if="editor.formatParams.type == 'a'">
+  <label>
+    <input ng-model="editor.formatParams.openLinkInCurrentTab" class="ng-pristine ng-valid ng-touched" type="checkbox">


There really isn't a reason to manually add these classes (ng-pristine, ng-valid and ng-touched). Angular will handle that automatically.

epixa · 2017-08-09T15:21:43Z

It looks like there are some linting issues caught by CI as well.

chrisronline · 2017-08-09T15:27:27Z

Good catch.

Copied from CI (which is linked below):

 /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-intake/src/core_plugins/kibana/common/field_formats/types/url.js
  109:13  error  'linkTarget' is never reassigned. Use 'const' instead  prefer-const
  110:1   error  Trailing spaces not allowed                            no-trailing-spaces

BigFunger

Couple small tweaks.

BigFunger · 2017-08-09T15:26:38Z

src/core_plugins/kibana/common/field_formats/types/url.js

@@ -106,7 +106,9 @@ UrlFormat.prototype._convert = {
          linkLabel = label;
        }

-        return `<a href="${url}" target="_blank">${linkLabel}</a>`;
+        let linkTarget = this.param('openLinkInCurrentTab') ? '_self' : '_blank';


const that up. :)

BigFunger · 2017-08-09T15:26:55Z

src/ui/public/field_format_editor/editors/url/url.html

@@ -119,6 +126,7 @@ <h4 class="hintbox-heading">
  </div>

  <input ng-model="editor.formatParams.labelTemplate" class="form-control">
+


Remove the extra whitespace, please.

BigFunger · 2017-08-09T15:28:59Z

src/ui/public/field_format_editor/editors/url/url.html

@@ -7,6 +7,13 @@
  </select>
 </div>

+<div class="checkbox" ng-if="editor.formatParams.type == 'a'">
+  <label>
+    <input ng-model="editor.formatParams.openLinkInCurrentTab" class="ng-pristine ng-valid ng-touched" type="checkbox">


These attributes should be one per line. See here for details.

fbaligand · 2017-08-09T15:56:00Z

Thanks for your feedback and your review !
I will fix all your remarks as soon as possible !

fbaligand · 2017-08-13T09:41:00Z

I'm currently on holidays (my turn ^^).
So I will fix remarks in about one week.
Sorry for the delay...

fbaligand · 2017-08-19T08:05:55Z

@epixa @chrisronline @BigFunger
I just fixed all your remarks and rebased my code to master.

=> Could you review it ?

If you wish I squash my 2 commits, tell me.

chrisronline · 2017-08-21T14:42:02Z

src/ui/public/field_format_editor/editors/url/url.html

@@ -8,6 +8,16 @@
  </select>
 </div>

+<div class="checkbox" ng-if="editor.formatParams.type == 'a'">
+  <label>


@cjcenizal @timroes Any accessibility concerns with this label/input approach?

@chrisronline nope, looks very fine :-) Thanks for pinging.

fbaligand · 2017-08-22T14:08:40Z

@chrisronline
For information, to write my html for checkbox, I used exactly the same style than this existing one :

kibana/src/core_plugins/console/public/src/directives/settings.html

Line 34 in 5514f34

chrisronline · 2017-08-22T15:06:14Z

src/ui/public/field_format_editor/editors/url/url.html

+  <label>
+    <input
+      type="checkbox"
+      ng-model="editor.formatParams.openLinkInCurrentTab">


Can we move the > to the next line? Along the lines of these examples

cjcenizal · 2017-08-22T15:26:07Z

src/ui/public/field_format_editor/editors/url/url.html

@@ -8,6 +8,16 @@
  </select>
 </div>

+<div class="checkbox" ng-if="editor.formatParams.type == 'a'">


Would you mind changing this to:

<div ng-if="editor.formatParams.type == 'a'"> <label class="kuiCheckBoxLabel"> <input class="kuiCheckBox" type="checkbox" ng-model="editor.formatParams.openLinkInCurrentTab" ></input> <span class="kuiCheckBoxLabel__text"> Open link in current tab </span> </label> </div>

These classes are provided by the UI Framework.

fbaligand · 2017-08-22T15:42:01Z

@chrisronline @cjcenizal
OK.
I just pushed a commit which fixes the checkbox html.

fbaligand · 2017-09-02T13:50:16Z

@chrisronline @BigFunger
Could you verify that your requested changes are done, and if so, give go for LGTM ?

chrisronline · 2017-09-02T15:04:05Z

LGTM!

fbaligand · 2017-09-02T15:05:57Z

Thanks @chrisronline !
Great news !

fbaligand · 2017-09-07T20:22:08Z

@timroes
Done !
=> Could you check it ?

timroes · 2017-09-07T20:25:52Z

LGTM @BigFunger would you care for an approval (or dismiss of your review :D)?

timroes · 2017-09-07T20:26:37Z

Jenkins, test this

timroes · 2017-09-07T21:42:53Z

Sorry, I think adding that rel=noopener broke now 3 tests in this test (line 10, 24 and 37 - also see jenkins failure). Could you add the rel=noopener there, too?

fbaligand · 2017-09-07T22:12:25Z

@timroes
Sorry for these broken tests.
I fixed them.

timroes · 2017-09-07T22:15:14Z

Jenkins, test this

fbaligand · 2017-09-08T11:45:00Z

@timroes
Jenkins says it's OK.
So now, what's the next step ?
Should we wait for @BigFunger review ?

BigFunger

LGTM! Sorry for the delay!

fbaligand · 2017-09-13T20:33:44Z

Great @BigFunger !

fbaligand · 2017-09-13T20:37:10Z

@timroes @BigFunger @chrisronline
Given that all checks are green, 3 reviewers validated the PR, could you merge the PR ?

…master branch) (#13209)

BigFunger · 2017-09-14T13:15:34Z

6.x/6.1: 5c93c1f
~~6.0: 7f85542~~ (revert: de4c1ba)

…master branch) (#13209)

fbaligand · 2017-09-14T13:23:25Z

Houhou ! Great !
Thanks @BigFunger !

…nt tab (master branch) (#13209)" This reverts commit 7f85542.

epixa · 2017-09-15T18:01:36Z

Thanks a bunch for this contribution @fbaligand!

fbaligand · 2017-09-15T18:47:57Z

@epixa happy to contribute !
With this PR, I now have contributed to Logstash, Elasticsearch and Kibana !

epixa · 2017-09-15T18:54:49Z

@fbaligand Nice!

fbaligand · 2017-09-15T20:17:10Z

By the way, I have another kibana PR ready to push !
A really nice feature up to me !
Teasing... :)

…master branch) (elastic#13209)

fbaligand changed the title ~~add an option for url types so that it can be opened in current tab~~ add an option for url types so that it can be opened in current tab (master branch) Jul 30, 2017

fbaligand mentioned this pull request Jul 30, 2017

add an option for url types so that it can be opened in current tab #13055

Closed

Bargs added :Management review labels Aug 7, 2017

This was referenced Aug 9, 2017

add an option for url types so that it can be opened in current tab (5.6 branch) #13210

Closed

add an option for url types so that it can be opened in current tab (6.0 branch) #13211

Closed

chrisronline suggested changes Aug 9, 2017

View reviewed changes

BigFunger suggested changes Aug 9, 2017

View reviewed changes

fbaligand force-pushed the open-link-in-current-tab branch from 9c87c9b to 204f44e Compare August 19, 2017 08:02

chrisronline reviewed Aug 21, 2017

View reviewed changes

chrisronline reviewed Aug 22, 2017

View reviewed changes

cjcenizal reviewed Aug 22, 2017

View reviewed changes

fbaligand added 3 commits August 25, 2017 23:02

add an option for url types so that it can be opened in current tab

6b5fd31

fix PR review remarks

b577074

fix PR review2 remarks

91ff72f

chrisronline approved these changes Sep 2, 2017

View reviewed changes

timroes approved these changes Sep 7, 2017

View reviewed changes

fix unit tests broked by rel=noopener

3238f74

BigFunger approved these changes Sep 13, 2017

View reviewed changes

BigFunger merged commit 7f40ab7 into elastic:master Sep 14, 2017

BigFunger pushed a commit that referenced this pull request Sep 14, 2017

add an option for url types so that it can be opened in current tab (…

5c93c1f

…master branch) (#13209)

BigFunger pushed a commit that referenced this pull request Sep 14, 2017

add an option for url types so that it can be opened in current tab (…

7f85542

…master branch) (#13209)

BigFunger added v6.1.0 v7.0.0 labels Sep 14, 2017

BigFunger added a commit that referenced this pull request Sep 14, 2017

Revert "add an option for url types so that it can be opened in curre…

de4c1ba

…nt tab (master branch) (#13209)" This reverts commit 7f85542.

epixa added the release_note:enhancement label Sep 15, 2017

fbaligand deleted the open-link-in-current-tab branch September 16, 2017 07:54

fbaligand added a commit to fbaligand/kibana that referenced this pull request Dec 17, 2017

add an option for url types so that it can be opened in current tab (…

18c0e5f

…master branch) (elastic#13209)

		@@ -119,6 +126,7 @@ <h4 class="hintbox-heading">
		</div>

		<input ng-model="editor.formatParams.labelTemplate" class="form-control">

add an option for url types so that it can be opened in current tab (master branch) #13209

add an option for url types so that it can be opened in current tab (master branch) #13209

Conversation

fbaligand commented Jul 30, 2017 • edited Loading

elasticmachine commented Jul 30, 2017

fbaligand commented Aug 5, 2017

epixa commented Aug 9, 2017

epixa commented Aug 9, 2017

chrisronline left a comment

Choose a reason for hiding this comment

chrisronline Aug 9, 2017

Choose a reason for hiding this comment

epixa commented Aug 9, 2017

chrisronline commented Aug 9, 2017

BigFunger left a comment

Choose a reason for hiding this comment

BigFunger Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

BigFunger Aug 9, 2017

Choose a reason for hiding this comment

BigFunger Aug 9, 2017

Choose a reason for hiding this comment

fbaligand commented Aug 9, 2017

fbaligand commented Aug 13, 2017

fbaligand commented Aug 19, 2017 • edited Loading

chrisronline Aug 21, 2017

Choose a reason for hiding this comment

timroes Aug 22, 2017

Choose a reason for hiding this comment

fbaligand commented Aug 22, 2017

chrisronline Aug 22, 2017

Choose a reason for hiding this comment

cjcenizal Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

fbaligand commented Aug 22, 2017 • edited Loading

fbaligand commented Sep 2, 2017

chrisronline commented Sep 2, 2017

fbaligand commented Sep 2, 2017

fbaligand commented Sep 7, 2017

timroes commented Sep 7, 2017

timroes commented Sep 7, 2017

timroes commented Sep 7, 2017

fbaligand commented Sep 7, 2017

timroes commented Sep 7, 2017

fbaligand commented Sep 8, 2017

BigFunger left a comment

Choose a reason for hiding this comment

fbaligand commented Sep 13, 2017

fbaligand commented Sep 13, 2017

BigFunger commented Sep 14, 2017 • edited Loading

fbaligand commented Sep 14, 2017

epixa commented Sep 15, 2017

fbaligand commented Sep 15, 2017

epixa commented Sep 15, 2017

fbaligand commented Sep 15, 2017

fbaligand commented Jul 30, 2017 •

edited

Loading

BigFunger Aug 9, 2017 •

edited

Loading

fbaligand commented Aug 19, 2017 •

edited

Loading

cjcenizal Aug 22, 2017 •

edited

Loading

fbaligand commented Aug 22, 2017 •

edited

Loading

BigFunger commented Sep 14, 2017 •

edited

Loading