Skip to content

[Ingest pipelines] Fixed 'Test pipeline' popover padding#99921

Merged
yuliacech merged 2 commits intoelastic:masterfrom
yuliacech:ingest_amsterdam
May 14, 2021
Merged

[Ingest pipelines] Fixed 'Test pipeline' popover padding#99921
yuliacech merged 2 commits intoelastic:masterfrom
yuliacech:ingest_amsterdam

Conversation

@yuliacech
Copy link
Copy Markdown
Contributor

@yuliacech yuliacech commented May 12, 2021 •
edited
Loading

Summary

Relates to #97699
This PR adds padding to 'Test pipeline' popover title and footer to fix the component for Amsterdam theme. In the Amsterdam theme, padding props affects all popover components (title, panel, footer).

Screenshots

Before

Screenshot 2021-05-12 at 14 17 26

After

Screenshot 2021-05-12 at 15 35 45

How to test

  1. Navigate to Ingest Node Pipelines app in Stack Management section.
  2. Create a pipeline and add any processor.
  3. In the pipeline form, click 'Add documents' button and past following documents into the JSON editor.
[
  {
    "_source": {
      "test": "test"
    }
  },
  {
    "_source": {
      "test": "test"
    }
  }
]
  1. Click 'Run the pipeline' button to add documents and close the flyout.
  2. In the pipeline form, click 'Document 1' link to open test documents popover.

@yuliacech yuliacech added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes Feature:Ingest Node Pipelines Ingest node pipelines management v7.14.0 labels May 12, 2021
@yuliacech yuliacech requested a review from a team as a code owner May 12, 2021 13:41
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 12, 2021

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@yuliacech yuliacech requested a review from sabarasaba May 12, 2021 13:45
@yuliacech yuliacech mentioned this pull request May 12, 2021
Closed
11 tasks
sabarasaba
sabarasaba approved these changes May 12, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@sabarasaba sabarasaba 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! I checked the docs from eui and also tested locally 👍🏼

Just one non-blocker thing that I noticed (unsure what are the politics about this kind of thing) but you can completely ignore if not relevant, is that the options rendered by the EuiSelectable component seem too tight together since it defaults to rowHeight: 32px I feel it might look a bit better if we make them slightly larger for example with: listProps={{ rowHeight: 38 }}

@yuliacech
Copy link
Copy Markdown
Contributor Author

yuliacech commented May 14, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 14, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ingestPipelines 680.7KB 680.7KB +26.0B
Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 4 2 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 5 3 -2
licensing 18 15 -3
monitoring 109 56 -53
total -73

History

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

@yuliacech
Copy link
Copy Markdown
Contributor Author

yuliacech commented May 14, 2021 •
edited
Loading

Thanks a lot for reviewing, @sabarasaba!
I tried out your suggestion for items' height but noticed that the overall selectable list height is hard coded to 208px so that fewer items will be visible at once. So I think we could leave the default height for now and open an issue if there is need to reconsider this design.

@yuliacech yuliacech merged commit df47ae1 into elastic:master May 14, 2021
@yuliacech yuliacech mentioned this pull request May 14, 2021
Merged
yuliacech added a commit that referenced this pull request May 14, 2021
@yuliacech @kibanamachine
Added missing padding to the popover title and footer in 'Test docume…
b51c48c 
…nts' popover (#99921) (#100126)

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

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
yctercero pushed a commit to yctercero/kibana that referenced this pull request May 25, 2021
@yuliacech @kibanamachine @yctercero
Added missing padding to the popover title and footer in 'Test docume…
d5e53a1 
…nts' popover (elastic#99921)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@yuliacech yuliacech deleted the ingest_amsterdam branch July 21, 2021 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sabarasaba sabarasaba sabarasaba approved these changes

Assignees

No one assigned

Labels

Feature:Ingest Node Pipelines Ingest node pipelines management release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v7.14.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yuliacech @elasticmachine @kibanamachine @sabarasaba