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

[Telemetry] Remove telemetry splash page and add conditional messaging #50189

Merged
merged 12 commits into from
Nov 12, 2019
Merged

[Telemetry] Remove telemetry splash page and add conditional messaging #50189

merged 12 commits into from
Nov 12, 2019

Conversation

joelgriffith
Copy link
Contributor

@joelgriffith joelgriffith commented Nov 11, 2019

Summary

Drops the telemetry splash page, removes most of the UI treatments but still leaves some services (client-side) intact just in case.

Screenie:

Screen Shot 2019-11-11 at 10 52 47 AM

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

@joelgriffith joelgriffith added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Nov 11, 2019
onDecline={this.onSampleDataDecline}
/>
<EuiSpacer size="s" />
<EuiTextColor className="euiText--small" color="subdued">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is a bit hectic, but I couldn't find a better mechanism for embedding links into a formatted message. Would love to know if folks have a better alternative as this is super noisy.

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

All looks good except the click here leads nowhere... so, that's obviously a blocker. :)

Comment on lines 137 to 140
<FormattedMessage
id="kbn.home.dataManagementDisableCollectionLink"
defaultMessage="click here."
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Clicking here just causes the page to refresh and ends up in the same place. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Same with the privacy policy link, which seems like it should go to https://www.elastic.co/legal/telemetry-privacy-statement , but perhaps we're waiting for updated links ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, these are just placeholders for now, thanks for calling it out!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added links!

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM it looks like it simplifies things a bit

Reviewed the code only

@tsullivan
Copy link
Member

@joelgriffith can you update the PR title to something more meaningful please? 😺

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm pretty unfamiliar with our UI bits (but now learning!)

Agree that the PR title should be changed tho :-)

@joelgriffith joelgriffith changed the title Chore/tel no splash page [Telemetry] Remove telemetry splash page and add conditional messaging Nov 12, 2019
Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@joelgriffith joelgriffith merged commit fada20d into elastic:master Nov 12, 2019
@alexfrancoeur
Copy link

LGTM overall. A few quick comments and questions.

image

Text is good, links work. @cchaos @ryankeairns this is super minor, but do we need more padding between the card and the usage data text?

Screen Shot 2019-11-12 at 4 37 04 PM

I realize we can't deep link directly, but is there any way to pre-populate the search bar with usage to filter down the list? I don't see any network calls being made and it seems to be purely UI based, but I thought I'd ask 😄

image

I tried loading with telemetry.allowChangingOptInStatus: false and received this bug in Chrome (incognito) with a fresh es snapshot. I'm pretty sure this config is in master. Has anyone else run into similar problems?

@alexfrancoeur
Copy link

Already merged 😄 I type too slow

@joelgriffith
Copy link
Contributor Author

@alexfrancoeur good catch on that issue, we did track that and need to alter our Joi validation. I'll follow up in another PR

jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 13, 2019
* upstream/master:
  Remove internal platform types exports (elastic#50427)
  [APM] Document `apm_oss.metricsIndices` and `apm_oss.sourcemap… (elastic#50312)
  [Telemetry] Server side fetcher (elastic#50015)
  [SIEM] Detection engine placeholders (elastic#50220)
  [Uptime] Donut chart loader position centered vertically  (elastic#50219)
  update telemetry banner notice text (elastic#50403)
  Fix aborting when searching without batching (elastic#49966)
  [Telemetry] Remove telemetry splash page and add conditional messaging (elastic#50189)
  Revert chromedriver update (elastic#50324)
  Remove deprecated argument include_type_name from ES calls (elastic#50285)
  [Maps] add settings to maps telemetry (elastic#50161)
  remove visualize loader (elastic#46910)
  Fix misuse of react-router and react-router-dom (elastic#50120)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 13, 2019
…-fallback

* 'master' of github.com:elastic/kibana:
  Remove internal platform types exports (elastic#50427)
  [APM] Document `apm_oss.metricsIndices` and `apm_oss.sourcemap… (elastic#50312)
  [Telemetry] Server side fetcher (elastic#50015)
  [SIEM] Detection engine placeholders (elastic#50220)
  [Uptime] Donut chart loader position centered vertically  (elastic#50219)
  update telemetry banner notice text (elastic#50403)
  Fix aborting when searching without batching (elastic#49966)
  [Telemetry] Remove telemetry splash page and add conditional messaging (elastic#50189)
  Revert chromedriver update (elastic#50324)
  Remove deprecated argument include_type_name from ES calls (elastic#50285)
  [Maps] add settings to maps telemetry (elastic#50161)
  remove visualize loader (elastic#46910)
  Fix misuse of react-router and react-router-dom (elastic#50120)
  Move table-list-view to kibana-react (elastic#50046)
  [ML] Stats bar for data frame analytics (elastic#49464)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 13, 2019
…ger-ace-theme

* 'master' of github.com:elastic/kibana: (56 commits)
  [ML] Server info service refactor (elastic#50302)
  Remove internal platform types exports (elastic#50427)
  [APM] Document `apm_oss.metricsIndices` and `apm_oss.sourcemap… (elastic#50312)
  [Telemetry] Server side fetcher (elastic#50015)
  [SIEM] Detection engine placeholders (elastic#50220)
  [Uptime] Donut chart loader position centered vertically  (elastic#50219)
  update telemetry banner notice text (elastic#50403)
  Fix aborting when searching without batching (elastic#49966)
  [Telemetry] Remove telemetry splash page and add conditional messaging (elastic#50189)
  Revert chromedriver update (elastic#50324)
  Remove deprecated argument include_type_name from ES calls (elastic#50285)
  [Maps] add settings to maps telemetry (elastic#50161)
  remove visualize loader (elastic#46910)
  Fix misuse of react-router and react-router-dom (elastic#50120)
  Move table-list-view to kibana-react (elastic#50046)
  [ML] Stats bar for data frame analytics (elastic#49464)
  [ML] Make navigation in tests more stable (elastic#50132)
  Migrate authorization subsystem to the new platform.  (elastic#46145)
  Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063)
  Update dependency @elastic/charts to v14 (elastic#49947)
  ...
@alexfrancoeur
Copy link

Closes #49518 when backported

@LeeDr
Copy link

LeeDr commented Nov 13, 2019

image

@tsullivan @gmmorris @pmuellr why are we approving PRs where none of these checkboxes are checked or striked-through?

joelgriffith pushed a commit that referenced this pull request Nov 13, 2019
…ssaging (#50189) (#50429)

* [Telemetry] Remove telemetry splash page and add conditional messaging (#50189)

* Removing tel splash page in UI layer

* Removing more components

* New disclaimer text

* Removing telemetry i18n text

* More i18n text removals

* Snapshot updates

* Snapshot tests + quick links for tel opt-out when possible

* Fixing TS issues in test

* i18n updates
joelgriffith pushed a commit that referenced this pull request Nov 13, 2019
…ssaging (#50189) (#50430)

* [Telemetry] Remove telemetry splash page and add conditional messaging (#50189)

* Removing tel splash page in UI layer

* Removing more components

* New disclaimer text

* Removing telemetry i18n text

* More i18n text removals

* Snapshot updates

* Snapshot tests + quick links for tel opt-out when possible

* Fixing TS issues in test

* Fixing broken telemetry updates

* [ML] Removes ChartTooltip component, artefact from backport gone wrong. (#50300)

* [7.5] [ML] Skip advanced wizard categorization test (#50141) (#50156)

* [ML] Skip advanced wizard categorization test (#50141)
* Re-enable other advanced tests

* [DOCS] Adds link to 7.5 breaking changes doc (#50496)

* Fixing mock interface in jest
@alexfrancoeur alexfrancoeur mentioned this pull request Nov 15, 2019
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants