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] Deangularize context.html #81442

Merged
merged 4 commits into from
Oct 27, 2020

Conversation

majagrubic
Copy link
Contributor

@majagrubic majagrubic commented Oct 22, 2020

Summary

Addresses: #70211

This is a continuation of the effort to deangularize context.html, by moving context-action-bar into ContextAppLegacy React component.

Checklist

Delete any items that are not applicable to this PR.

- [ ] 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

For maintainers

@majagrubic majagrubic added v7.11.0 Feature:Discover Discover Application Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes labels Oct 22, 2020
@majagrubic majagrubic marked this pull request as ready for review October 22, 2020 11:01
@majagrubic majagrubic requested a review from a team October 22, 2020 11:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)


export interface ContextAppProps {
export type ContextAppProps = Partial<ActionBarProps> & {
Copy link
Member

Choose a reason for hiding this comment

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

Since only defaultStepSize of ActionBarProps seems to be used, it might make sense to add this to ContextAppProps to make this explicit and interface it

const PREDECESSOR_TYPE = 'predecessors';
const SUCCESSOR_TYPE = 'successors';

function isPredecessorType(type: string) {
Copy link
Member

@kertal kertal Oct 22, 2020

Choose a reason for hiding this comment

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

Since isPredecessorType is only used once, you could simplify by migrate it to the actionBarProps function like const isPredecessorType = isPredecessorType;

Copy link
Member

Choose a reason for hiding this comment

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

I'm sure you won't do it like this const isPredecessorType = isPredecessorType; 🤦

Copy link
Member

@kertal kertal 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, tested locally in Chrome, Firefox, Safari, works as expected 👍 , just left a few thoughts about code that might be a question of taste

@majagrubic
Copy link
Contributor Author

@elastimachine merge upstream

@majagrubic
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
discover 439.1KB 439.6KB +452.0B

History

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

@majagrubic majagrubic merged commit 3228f71 into elastic:master Oct 27, 2020
@majagrubic majagrubic deleted the deangularize-context-2 branch October 27, 2020 12:57
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 27, 2020
* master:
  [Security Solution][Endpoint][Admin] Malware Protections Notify User Version (elastic#81415)
  Revert "[Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)"
  [Maps] Use default format when proxying EMS-files (elastic#79760)
  [Discover] Deangularize context.html (elastic#81442)
  Use the displayName property in default editor (elastic#73311)
  adds bug label to Bug report for Security Solution template (elastic#81643)
  [ML] Transforms: Remove index field limitation for custom query. (elastic#81467)
  [Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)
  [Task Manager] Mark task as failed if maxAttempts has been met. (elastic#80681)
  [Lens] Fix URL query loss on redirect (elastic#81475)
  Log reason for 404 in field existence check (elastic#81315)
majagrubic pushed a commit that referenced this pull request Oct 27, 2020
* [Discover] Deangularize context.html

* Removing snapshot testing

* Applying PR comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 27, 2020
* master: (87 commits)
  [Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81778)
  [i18n] add get_kibana_translation_paths tests (elastic#81624)
  [UX] Fix search term reset from url (elastic#81654)
  [Lens] Improved range formatter (elastic#80132)
  [Resolver] `SideEffectContext` changes, remove `@testing-library` uses (elastic#81077)
  [Time to Visualize] Update Library Text with Call to Action (elastic#81667)
  [docs] Resolving failed Kibana upgrade migrations (elastic#80999)
  [ftr/menuToggle] provide helper for enhanced menu toggle handling (elastic#81709)
  [Task Manager] adds basic observability into Task Manager's runtime operations (elastic#77868)
  Doc changes for stack management and grouped feature privileges (elastic#80486)
  Added functional test for alerts list filters by status, alert type and action type. Did a code refactoring and splitting for alerts tests. (elastic#81422)
  [Security Solution][Endpoint][Admin] Malware Protections Notify User Version (elastic#81415)
  Revert "[Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)"
  [Maps] Use default format when proxying EMS-files (elastic#79760)
  [Discover] Deangularize context.html (elastic#81442)
  Use the displayName property in default editor (elastic#73311)
  adds bug label to Bug report for Security Solution template (elastic#81643)
  [ML] Transforms: Remove index field limitation for custom query. (elastic#81467)
  [Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)
  [Task Manager] Mark task as failed if maxAttempts has been met. (elastic#80681)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants