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

[Discover] Inline angular directives only used in this plugin #56119

Conversation

kertal
Copy link
Member

@kertal kertal commented Jan 28, 2020

Summary

The following imports are only used by discover and are inlined into the discover plugin by this PR

  • ui/directives/field_name/field_name
  • ui/directives/listen/listen - it's also used by timelion and maps
  • ui/collapsible_sidebar/collapsible_sidebar
  • ui/directives/css_truncate
  • ui/fixed_scroll
  • ui/directives/debounce/debounce
  • ui/render_complete/directive

Part of #50670

Checklist

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

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials
- [ ] Unit or functional tests were updated or added to match the most common scenarios
- [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@kertal kertal self-assigned this Jan 28, 2020
@kertal kertal changed the title [Discover] Inline angular directive only used here [Discover] Inline angular directives only used in this plugin Jan 28, 2020
@kertal kertal added Feature:NP Migration Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Jan 29, 2020
@kertal kertal marked this pull request as ready for review January 30, 2020 08:35
@kertal kertal requested a review from a team January 30, 2020 08:35
@kertal kertal requested a review from a team as a code owner January 30, 2020 08:35
@import 'histogram';
@import './collapsible_sidebar/index';
Copy link
Member Author

Choose a reason for hiding this comment

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

dear @elastic/kibana-design, no new SCSS inside, just change of path and imports

Copy link
Contributor

Choose a reason for hiding this comment

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

I just reviewed the SCSS files that were moved to a new directory and LGTM!

@kertal
Copy link
Member Author

kertal commented Jan 30, 2020

Jenkins, test this - seems to be a unrelated error

@kertal
Copy link
Member Author

kertal commented Jan 30, 2020

@elasticmachine merge upstream

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested and everything still seems to work - LGTM

@@ -19,8 +19,8 @@
import React from 'react';
import classNames from 'classnames';
// @ts-ignore
import { shortenDottedString } from '../../../../core_plugins/kibana/common/utils/shorten_dotted_string';
import { FieldIcon } from '../../../../../../src/plugins/kibana_react/public';
import { shortenDottedString } from '../../../../../../common/utils/shorten_dotted_string';
Copy link
Contributor

@flash1293 flash1293 Jan 31, 2020

Choose a reason for hiding this comment

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

It seems like shorten_dotted_string is also available from the data plugin src/plugins/data/common/utils/shorten_dotted_string.ts

Copy link
Member Author

@kertal kertal Jan 31, 2020

Choose a reason for hiding this comment

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

Thanks for that hint! Done! Goodbye to another @ts-ignore, another brick in our old-kibana-wall. think we could now start to plan a museum of old code fragments. before everybody forgets.

…of github.com:kertal/kibana into kertal-pr-2020-01-28-discover_inline_static_code_deps
@kertal
Copy link
Member Author

kertal commented Feb 4, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kertal
Copy link
Member Author

kertal commented Feb 4, 2020

thx @miukimiu, @flash1293 for the reviews, time to merge!

@kertal kertal merged commit d73e15f into elastic:master Feb 4, 2020
kertal added a commit to kertal/kibana that referenced this pull request Feb 4, 2020
…c#56119)

* Migrate field_name directive

* Migrate collapsible_sidebar directive

* Fix FieldName import at table_row.tsx

* Migrate css_truncate directive

* Migrate fixed_scroll & debounce directives

* Migrate render_complete directive

* Fix css_truncate test

* Use shortenDottedString in the TypesScript version
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 4, 2020
* master: (42 commits)
  Move kuery_autocomplete ⇒ NP (elastic#56607)
  [ML] Functional tests - stabilize job row and analytics result view assertions (elastic#56595)
  [Discover] Inline angular directives only used in this plugin (elastic#56119)
  [Discover] Migrate get_sort.js test from mocha to TypeScript (elastic#56011)
  [SIEM] Enable flow_target_select_connected unit tests (elastic#55618)
  Start consuming np logging config (elastic#56480)
  [SIEM] Add eslint-plugin-react-perf (elastic#55960)
  Mention changed SAML ACS endpoint URL in breaking changes doc. (elastic#56613)
  Add `getServerInfo` API to http setup contract (elastic#56636)
  Updates Monitoring alert Jest snapshots
  Kibana property config migrations (elastic#55937)
  Vislib replacement toggle (elastic#56439)
  [Uptime] Add unit tests for QueryContext time calculation (elastic#56671)
  [SIEM][Detection Engine] Critical blocker, fixes pre-packaged rule miscounts
  Upgrade EUI to v18.3.0 (elastic#56228)
  [Maps] Fix server log (elastic#56679)
  [SIEM] Fixes FTUE when APM node is present (elastic#56574)
  [Reporting/FieldFormats] expose `setFieldFormats` and call from ReportingPlugin.start (elastic#56563)
  Update EMS API urls for production (elastic#56657)
  Ability to delete alerts even when AAD is out of sync (elastic#56543)
  ...
kertal added a commit that referenced this pull request Feb 4, 2020
#56744)

* Migrate field_name directive

* Migrate collapsible_sidebar directive

* Fix FieldName import at table_row.tsx

* Migrate css_truncate directive

* Migrate fixed_scroll & debounce directives

* Migrate render_complete directive

* Fix css_truncate test

* Use shortenDottedString in the TypesScript version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants