Skip to content

Adding attributes to be used by sharing#10234

Merged
kobelb merged 8 commits intoelastic:masterfrom
kobelb:shared-attributes
Feb 17, 2017
Merged

Adding attributes to be used by sharing#10234
kobelb merged 8 commits intoelastic:masterfrom
kobelb:shared-attributes

Conversation

@kobelb
Copy link
Copy Markdown
Contributor

@kobelb kobelb commented Feb 7, 2017

Items that will be shared are specifically labeled now, and they provide a title/description via an attribute.

@kobelb kobelb added :Sharing Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Feb 7, 2017
epixa
epixa reviewed Feb 7, 2017
View reviewed changes
Comment thread src/core_plugins/kibana/public/dashboard/panel/panel.html Outdated
Copy link
Copy Markdown
Contributor

@epixa epixa Feb 7, 2017

Choose a reason for hiding this comment

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

Typo?

Copy link
Copy Markdown
Contributor Author

@kobelb kobelb Feb 7, 2017

Choose a reason for hiding this comment

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

Yup, thanks!

@kobelb
Copy link
Copy Markdown
Contributor Author

kobelb commented Feb 7, 2017

@ppisljar I removed the shared-item- prefix from title/description under the assumption that these would be relevant to the more general visualizations API. If you'd prefer these be moved back to the shared-item- 'namespace' just lemme know.

@kobelb kobelb requested a review from ppisljar February 7, 2017 21:32
ppisljar
ppisljar approved these changes Feb 8, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM, however i did like @epixa comment in the previous PR to name it object (object-title, object-description and object-something ?)

@kobelb
Copy link
Copy Markdown
Contributor Author

kobelb commented Feb 8, 2017 •
edited
Loading

@ppisljar I'm fine with changing title/description/etc. to object-title/object-description/etc. if that is more inline what you guys were thinking for the visualization API. However, I'd like to keep shared-item as opposed to object-item.

@ppisljar
Copy link
Copy Markdown
Contributor

ppisljar commented Feb 8, 2017

i'll let you and @thomasneirynck decide

@Bargs Bargs self-requested a review February 8, 2017 15:00
@epixa
Copy link
Copy Markdown
Contributor

epixa commented Feb 8, 2017

Can you either update existing selenium tests or create new ones that verify these attributes in some way? These effectively become part of our plugin API, so we should have tests to prevent regressions.

@kobelb
Copy link
Copy Markdown
Contributor Author

kobelb commented Feb 8, 2017

@epixa good call, I will do so.

stacey-gammon
stacey-gammon reviewed Feb 8, 2017
View reviewed changes
Comment thread src/core_plugins/kibana/public/dashboard/panel/panel.html Outdated
Copy link
Copy Markdown

@stacey-gammon stacey-gammon Feb 8, 2017

Choose a reason for hiding this comment

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

I wonder if we could use consts somehow, so that way if they are ever changed in kibana, it won't break reporting. But I guess you can't create attributes dynamically, like

<visualize {{Consts.SharedItemTag}} />

Similar to your worry when I fixed the issue with hiding the reporting link on the dashboard landing page. It's going to be hard for someone to realize that these attributes are used in reporting and changing them will break it. Especially if they are here to help a timing issue, knowing when to load the page, tests might not even fail right away.

I wonder if we could use a const if we did something similar to how we use data-test-subj. Though that only works if no one changes the data-test-subj tag itself.

I just could see someone being like, why is this here and what does it do, and it's not very discoverable.

Copy link
Copy Markdown
Contributor Author

@kobelb kobelb Feb 8, 2017

Choose a reason for hiding this comment

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

At one point, these were calling reporting-item to be very explicit towards their use; however, we switched the name to be more general because they're technically not just for reporting.

@Bargs
Copy link
Copy Markdown
Contributor

Bargs commented Feb 8, 2017

I'm fine with changing title/description/etc. to object-title/object-description/etc.

I would recommend this, since title is a valid html attribute that I don't think we want to use. The way it's used right now, the saved search title appears when you hover over any part of the doc table in discover. I'm also a proponent of using the data- prefix.

@kobelb
Copy link
Copy Markdown
Contributor Author

kobelb commented Feb 8, 2017

@Bargs great catch on the conflict with the HTML title attribute. It seems like the data- prefix has become 'kibana standard', anyone have an objection to renaming them to data-title and data-description?

@Bargs
Copy link
Copy Markdown
Contributor

Bargs commented Feb 8, 2017

Also an html standard ;)

thomasneirynck
thomasneirynck reviewed Feb 8, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

agree, I'd change it to data-*, since that how data-attributes are recommended to be used (per @Bargs)

Other than that, LGTM

@tbragin tbragin added the review label Feb 13, 2017
@kobelb kobelb merged commit 2fb2b4c into elastic:master Feb 17, 2017
@elastic-jasper elastic-jasper mentioned this pull request Feb 17, 2017
Merged
elastic-jasper added a commit that referenced this pull request Feb 17, 2017
@elastic-jasper
Adding attributes to be used by sharing
0ad713e 
Backports PR #10234

**Commit 1:**
Adding attributes to be used by sharing

* Original sha: cbb5047
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-01-25T15:53:10Z

**Commit 2:**
Moving the shared-item tags for dashboard panels

* Original sha: ba4b74b
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-07T20:27:36Z

**Commit 3:**
Switching from shared-item-title/description to just title/description

* Original sha: 757d7cf
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-07T20:31:09Z

**Commit 4:**
Removing move noise

* Original sha: c54df9f
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-07T20:33:19Z

**Commit 5:**
Fixing typo

* Original sha: 7a13bc9
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-07T20:34:18Z

**Commit 6:**
Adding data- prefix before attributes, began writing tests

Adding shared-item title tests

Switching up the panel tests

Adding description tests

Fixing linting

Adding dashboard timefilter test

Adding visualization shared-item tests

Adding shared-timefilter tests

Adding discover shared-item tests

* Original sha: 0dbb7eb
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-09T19:39:13Z

**Commit 7:**
Fixing shared-items-count on dashboard

* Original sha: 7ab7a70
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-16T14:25:58Z

**Commit 8:**
Fixing functional test data for discover and visualize

* Original sha: 063a49e
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-16T20:57:22Z
kobelb pushed a commit that referenced this pull request Feb 17, 2017
@elastic-jasper @kobelb
Adding attributes to be used by sharing (#10425)
9b8e401 
Backports PR #10234

**Commit 1:**
Adding attributes to be used by sharing

* Original sha: cbb5047
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-01-25T15:53:10Z

**Commit 2:**
Moving the shared-item tags for dashboard panels

* Original sha: ba4b74b
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-07T20:27:36Z

**Commit 3:**
Switching from shared-item-title/description to just title/description

* Original sha: 757d7cf
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-07T20:31:09Z

**Commit 4:**
Removing move noise

* Original sha: c54df9f
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-07T20:33:19Z

**Commit 5:**
Fixing typo

* Original sha: 7a13bc9
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-07T20:34:18Z

**Commit 6:**
Adding data- prefix before attributes, began writing tests

Adding shared-item title tests

Switching up the panel tests

Adding description tests

Fixing linting

Adding dashboard timefilter test

Adding visualization shared-item tests

Adding shared-timefilter tests

Adding discover shared-item tests

* Original sha: 0dbb7eb
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-09T19:39:13Z

**Commit 7:**
Fixing shared-items-count on dashboard

* Original sha: 7ab7a70
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-16T14:25:58Z

**Commit 8:**
Fixing functional test data for discover and visualize

* Original sha: 063a49e
* Authored by kobelb <brandon.kobel@elastic.co> on 2017-02-16T20:57:22Z
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ppisljar ppisljar ppisljar approved these changes

+4 more reviewers

@epixa epixa epixa left review comments

@thomasneirynck thomasneirynck thomasneirynck left review comments

@stacey-gammon stacey-gammon stacey-gammon left review comments

@Bargs Bargs Bargs approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@kobelb @ppisljar @epixa @Bargs @thomasneirynck @stacey-gammon @tbragin