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.

Sign up for GitHub

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

Jump to bottom

[Inspector Views] [Request View] - Migrate inspector_views to new platform #43191

Merged
merged 8 commits into from
Aug 19, 2019
Merged

[Inspector Views] [Request View] - Migrate inspector_views to new platform #43191

merged 8 commits into from
Aug 19, 2019

Conversation

alexwizp
Copy link
Contributor

Partially fix: #42636

Description:

There is still an inspector_views plugin in src/legacy/core_plugins/inspector_views which should be colocated with the inspector plugin to complete the migration.

Done for Request View

Checklist

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

For maintainers

@elastic elastic deleted a comment from elasticmachine Aug 13, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@alexwizp alexwizp marked this pull request as ready for review August 14, 2019 07:56
@alexwizp alexwizp requested a review from a team as a code owner August 14, 2019 07:56
@alexwizp alexwizp self-assigned this Aug 14, 2019
@alexwizp alexwizp added v7.4.0 v8.0.0 Feature:Inspector Inspector infrastructure and implementations Team:AppArch labels Aug 14, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@alexwizp alexwizp added release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration labels Aug 14, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

ryankeairns
ryankeairns approved these changes Aug 14, 2019
View reviewed changes
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

LGTM from the design side. The CSS changes do not affect actual styles, just some file renaming and import changes.

cchaos
cchaos suggested changes Aug 14, 2019
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

@alexwizp The only problem is that you renamed _requests.scss to index.scss. There's two problems with this.

  1. Imported (not directly compiled) file names should always lead with an underscore.
  2. We never put styles directly in index files. These should strictly be import manifests.

Solution

  1. Revert the rename of _requests.scss.
  2. Add back in an _index.scss file next to the requests that imports it @import './requests
  3. Rename this index.scss file to _index.scss since it's being imported from the original location and is not a directly compiled file.

@alexwizp alexwizp requested a review from cchaos August 14, 2019 16:46
@alexwizp
Copy link
Contributor Author

@cchaos thanks, fixed

cchaos
cchaos approved these changes Aug 14, 2019
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thx @alexwizp

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alexwizp
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

lukeelmers
lukeelmers approved these changes Aug 16, 2019
View reviewed changes
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Thanks @alexwizp! Code LGTM, added a few minor notes, but no major concerns on my end.

Before merging, I'd like to get an approval from either @streamich or @timroes as well, since they are more familiar with this code than I am.

src/plugins/inspector/public/views/requests/components/details/req_details_stats.tsx Show resolved Hide resolved
src/plugins/inspector/public/views/requests/components/details/req_details_stats.tsx Show resolved Hide resolved
src/plugins/inspector/public/views/requests/components/requests_view.tsx Show resolved Hide resolved
src/plugins/inspector/public/views/requests/index.ts Outdated Show resolved Hide resolved
src/plugins/inspector/public/views/requests/components/details/req_details_request.tsx Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elastic elastic deleted a comment from elasticmachine Aug 16, 2019
@alexwizp
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukeelmers lukeelmers mentioned this pull request Aug 19, 2019
Closed
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

streamich
streamich approved these changes Aug 19, 2019
View reviewed changes
Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

Code LGTM, but I only see Request view being moved there should be also Data view.

@alexwizp alexwizp merged commit 9f0de8d into elastic:master Aug 19, 2019
alexwizp added a commit to alexwizp/kibana that referenced this pull request Aug 19, 2019
@alexwizp
[Inspector Views] [Request View] - Migrate inspector_views to new pla…
7f60212 
…tform (elastic#43191)

* [Inspector Views] [Request View] - Migrate inspector_views to new platform

* fix i18n keys

* rename scss files

* fix PR comments
@alexwizp alexwizp mentioned this pull request Aug 19, 2019
Merged
alexwizp added a commit that referenced this pull request Aug 19, 2019
@alexwizp
[Inspector Views] [Request View] - Migrate inspector_views to new pla…
be7aa6a 
…tform (#43191) (#43518)

* [Inspector Views] [Request View] - Migrate inspector_views to new platform

* fix i18n keys

* rename scss files

* fix PR comments
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 19, 2019
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into chore/console…
98e12c2 
…_update_json_spec

* 'master' of github.com:elastic/kibana: (35 commits)
  fix: 🐛 pass whole action context to isCompatible() method (elastic#43457)
  Deleted old kbn-top-nav directive (elastic#43168)
  [ML] Fixing cloning of single metric distinct count job (elastic#43435)
  Update @elastic/charts version 8.1.6 > 9.1.1 (elastic#43516)
  [Inspector Views] [Request View] - Migrate inspector_views to new platform (elastic#43191)
  [ML] Adding loading indicators to all wizard charts (elastic#43382)
  disable flaky test (elastic#43492)
  feature(code/frontend): cancel file blob and directory commits request if outdated (elastic#43348)
  fix(code/frontend): button group url should have previous query string (elastic#43428)
  [SIEM] Fixes index substring incorrectly matching configured indices and failing to install ML job (elastic#43409)
  [SIEM] Adds performance enhancements such by removing wasted renderers and adding incremental DOM rendering (elastic#43157)
  disable flaky test (elastic#37859)
  Added sass lint to Canvas (elastic#43410)
  [Maps] add indicator when layer is filtered by search bar (elastic#43283)
  Properly validate current user password during password change. (elastic#43447)
  Spaces - allow for hex color codes that include uppercase characters (elastic#43470)
  [Reporting] Add a bit more logging and a few more logging level promotions (elastic#43415)
  Partially convert index pattern server to typescript (elastic#43291)
  [Infra UI] Use sum for aggregating AWS metrics. (elastic#43293)
  [SIEM] Format bytes columns in timeline (elastic#43147)
  ...
@alexwizp alexwizp deleted the inspector_request_view branch January 4, 2020 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@streamich streamich streamich approved these changes

@lukeelmers lukeelmers lukeelmers approved these changes

@cchaos cchaos cchaos approved these changes

@ryankeairns ryankeairns ryankeairns approved these changes

@lizozom lizozom Awaiting requested review from lizozom

@timroes timroes Awaiting requested review from timroes

Assignees

@alexwizp alexwizp

Labels
Feature:Inspector Inspector infrastructure and implementations Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.4.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate inspector_views to new platform
6 participants
@alexwizp @elasticmachine @ryankeairns @cchaos @lukeelmers @streamich