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

[Security Solution] Timeline UI refactor revision - Design feedback #173015

Merged
merged 17 commits into from
Dec 13, 2023

Conversation

logeekal
Copy link
Contributor

@logeekal logeekal commented Dec 11, 2023

Summary

This PR implements Design feedback for the Timeline UI refactoring:

  1. Adds ⨁ back in timeline bottom bar.
    image
  2. Add Unsaved badge for better visibility
    image ===> image
  3. Adds a new tour step for Add to Favorites Icon.
Screen.Recording.2023-12-11.at.08.06.56.mov

Checklist

Delete any items that are not applicable to this PR.

@logeekal logeekal added the Team:Threat Hunting:Investigations Security Solution Investigations Team label Dec 11, 2023
@logeekal logeekal self-assigned this Dec 11, 2023
@logeekal logeekal marked this pull request as ready for review December 11, 2023 07:18
@logeekal logeekal requested review from a team as code owners December 11, 2023 07:18
@logeekal logeekal added release_note:skip Skip the PR/issue when compiling release notes v8.12.0 labels Dec 11, 2023
Copy link
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

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

Awesome work! LGTM 👍🏾

@logeekal logeekal added ci:cloud-deploy Create or update a Cloud deployment ci:project-persist-deployment Persist project deployment indefinitely labels Dec 11, 2023
@logeekal logeekal added the ci:cloud-redeploy Always create a new Cloud deployment label Dec 11, 2023
Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

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

Left a few minor nits.

I feel like we could have a small space between the new badge and the timeline title? A little bit like we have between the favorite start icon and the title on the left? It feels a bit close to me at the moment
Screenshot 2023-12-12 at 11 37 48 AM

I also noticed once the Unsaved badge flickering during save but it does not happen all the time.I'm thinking it's some sort of raise condition issue, but probably pretty minor...

Screen.Recording.2023-12-12.at.11.40.44.AM.mov

@logeekal logeekal removed the ci:cloud-redeploy Always create a new Cloud deployment label Dec 13, 2023
@logeekal
Copy link
Contributor Author

logeekal commented Dec 13, 2023

@PhilippeOberti

I feel like we could have a small space between the new badge and the timeline title? A little bit like we have between the favorite start icon and the title on the left? It feels a bit close to me at the moment

Screenshot 2023-12-12 at 11 37 48 AM

You know I have been struggling with similar dilemma. But if you notice below screenshot, the space (size s) is uniform between all elements but the different padding in the icons creates that illusion. I just did not want to have too many custom CSS within icons.
image

image

I also noticed once the Unsaved badge flickering during save but it does not happen all the time.I'm thinking it's some sort of raise condition issue, but probably pretty minor...

Yes it is because of how Discover saved search works.. It is not the smoothest experience. But as we revamp ESQL tab, it should be resolved soon.

@@ -41,8 +41,8 @@ const AddTimelineButtonComponent: React.FC<AddTimelineButtonComponentProps> = ({
className={ADD_TIMELINE_BUTTON_CLASS_NAME}
data-test-subj="settings-plus-in-circle"
Copy link
Contributor

Choose a reason for hiding this comment

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

If you end up updating the PR, mind giving this a more timeline related data-test-subj? Something like new-timeline-header-action or something specific to timeline that doesn't conflict with the existing new-timeline-action? No worries, if you don't make any additional changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure @michaelolo24 , I will check the impact on test, if it is not that big. I will go ahead and change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelolo24 What do you think about timeline-create-open-control?

Copy link
Contributor

Choose a reason for hiding this comment

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

Works perfectly, thanks!

Copy link
Contributor

@nastasha-solomon nastasha-solomon 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 a copy perspective - thank you for your hard work on this, @logeekal!

@logeekal logeekal enabled auto-merge (squash) December 13, 2023 18:53
@kibana-ci
Copy link
Collaborator

kibana-ci commented Dec 13, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

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
securitySolution 13.1MB 13.1MB +13.1KB

History

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

cc @logeekal

Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

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

LGTM!

@logeekal logeekal merged commit 1ba247e into elastic:main Dec 13, 2023
39 checks passed
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.12 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 173015

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 17, 2023
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 173015 locally

@logeekal
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.12

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

logeekal added a commit to logeekal/kibana that referenced this pull request Dec 18, 2023
…lastic#173015)

## Summary

This PR implements Design feedback for the Timeline UI refactoring:

1. Adds ⨁ back in timeline bottom bar.

![image](https://github.com/elastic/kibana/assets/7485038/1572527a-471b-4edb-826a-29610eae0a4a)
2. Add `Unsaved` badge for better visibility

![image](https://github.com/elastic/kibana/assets/7485038/b1baf745-fcda-4346-a104-a78b88853ce4)
===>
![image](https://github.com/elastic/kibana/assets/7485038/9d279276-cc86-44d4-903c-53ebe60e55fb)
3. Adds a new tour step for `Add to Favorites` Icon.

https://github.com/elastic/kibana/assets/7485038/2d4f3e2e-7868-4fbb-8a73-c9b427d86e04

### Checklist

Delete any items that are not applicable to this PR.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed

(cherry picked from commit 1ba247e)

# Conflicts:
#	x-pack/test/security_solution_cypress/cypress/e2e/investigations/timelines/creation.cy.ts
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 18, 2023
logeekal added a commit that referenced this pull request Dec 18, 2023
…dback (#173015) (#173498)

# Backport

This will backport the following commits from `main` to `8.12`:
- [[Security Solution] Timeline UI refactor revision - Design feedback
(#173015)](#173015)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Jatin
Kathuria","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-12-13T23:17:19Z","message":"[Security
Solution] Timeline UI refactor revision - Design feedback
(#173015)\n\n## Summary\r\n\r\nThis PR implements Design feedback for
the Timeline UI refactoring:\r\n\r\n1. Adds ⨁ back in timeline bottom
bar.\r\n\r\n![image](https://github.com/elastic/kibana/assets/7485038/1572527a-471b-4edb-826a-29610eae0a4a)\r\n2.
Add `Unsaved` badge for better
visibility\r\n\r\n![image](https://github.com/elastic/kibana/assets/7485038/b1baf745-fcda-4346-a104-a78b88853ce4)\r\n===>\r\n![image](https://github.com/elastic/kibana/assets/7485038/9d279276-cc86-44d4-903c-53ebe60e55fb)\r\n3.
Adds a new tour step for `Add to Favorites`
Icon.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/7485038/2d4f3e2e-7868-4fbb-8a73-c9b427d86e04\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [x] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[x]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests
changed","sha":"1ba247ee719565ce3dd345ea0138a60bd71a6656","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport
missing","Team:Threat
Hunting:Investigations","ci:cloud-deploy","v8.12.0","ci:project-persist-deployment","v8.13.0"],"number":173015,"url":"https://github.com/elastic/kibana/pull/173015","mergeCommit":{"message":"[Security
Solution] Timeline UI refactor revision - Design feedback
(#173015)\n\n## Summary\r\n\r\nThis PR implements Design feedback for
the Timeline UI refactoring:\r\n\r\n1. Adds ⨁ back in timeline bottom
bar.\r\n\r\n![image](https://github.com/elastic/kibana/assets/7485038/1572527a-471b-4edb-826a-29610eae0a4a)\r\n2.
Add `Unsaved` badge for better
visibility\r\n\r\n![image](https://github.com/elastic/kibana/assets/7485038/b1baf745-fcda-4346-a104-a78b88853ce4)\r\n===>\r\n![image](https://github.com/elastic/kibana/assets/7485038/9d279276-cc86-44d4-903c-53ebe60e55fb)\r\n3.
Adds a new tour step for `Add to Favorites`
Icon.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/7485038/2d4f3e2e-7868-4fbb-8a73-c9b427d86e04\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [x] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[x]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests
changed","sha":"1ba247ee719565ce3dd345ea0138a60bd71a6656"}},"sourceBranch":"main","suggestedTargetBranches":["8.12"],"targetPullRequestStates":[{"branch":"8.12","label":"v8.12.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.13.0","labelRegex":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/173015","number":173015,"mergeCommit":{"message":"[Security
Solution] Timeline UI refactor revision - Design feedback
(#173015)\n\n## Summary\r\n\r\nThis PR implements Design feedback for
the Timeline UI refactoring:\r\n\r\n1. Adds ⨁ back in timeline bottom
bar.\r\n\r\n![image](https://github.com/elastic/kibana/assets/7485038/1572527a-471b-4edb-826a-29610eae0a4a)\r\n2.
Add `Unsaved` badge for better
visibility\r\n\r\n![image](https://github.com/elastic/kibana/assets/7485038/b1baf745-fcda-4346-a104-a78b88853ce4)\r\n===>\r\n![image](https://github.com/elastic/kibana/assets/7485038/9d279276-cc86-44d4-903c-53ebe60e55fb)\r\n3.
Adds a new tour step for `Add to Favorites`
Icon.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/7485038/2d4f3e2e-7868-4fbb-8a73-c9b427d86e04\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [x] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[x]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests
changed","sha":"1ba247ee719565ce3dd345ea0138a60bd71a6656"}}]}]
BACKPORT-->
janmonschke added a commit that referenced this pull request Jan 16, 2024
janmonschke pushed a commit to janmonschke/kibana that referenced this pull request Jan 16, 2024
…173308)

Dependent on: elastic#173015

- fixes elastic#165663
- fixes elastic#165747

[Flaky Tests
(100/100)](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4870)

---------

Co-authored-by: Jan Monschke <[email protected]>
(cherry picked from commit 1172c0e)

# Conflicts:
#	x-pack/test/security_solution_cypress/cypress/tasks/common.ts
janmonschke referenced this pull request Jan 17, 2024
…173308) (#174914)

# Backport

This will backport the following commits from `main` to `8.12`:
- [[Investigations] - Unskip and refactor discover state tests
(#173308)](#173308)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Michael
Olorunnisola","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-01-16T12:18:26Z","message":"[Investigations]
- Unskip and refactor discover state tests (#173308)\n\nDependent on:
https://github.com/elastic/kibana/pull/173015\r\n\r\n- fixes
https://github.com/elastic/kibana/issues/165663\r\n- fixes
https://github.com/elastic/kibana/issues/165747\r\n\r\n[Flaky
Tests\r\n(100/100)](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4870)\r\n\r\n---------\r\n\r\nCo-authored-by:
Jan Monschke
<[email protected]>","sha":"1172c0ec09eb72f478b7c0cb7afc6016fabdc400","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Threat
Hunting:Investigations","v8.12.1","v8.13.0"],"number":173308,"url":"https://github.com/elastic/kibana/pull/173308","mergeCommit":{"message":"[Investigations]
- Unskip and refactor discover state tests (#173308)\n\nDependent on:
https://github.com/elastic/kibana/pull/173015\r\n\r\n- fixes
https://github.com/elastic/kibana/issues/165663\r\n- fixes
https://github.com/elastic/kibana/issues/165747\r\n\r\n[Flaky
Tests\r\n(100/100)](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4870)\r\n\r\n---------\r\n\r\nCo-authored-by:
Jan Monschke
<[email protected]>","sha":"1172c0ec09eb72f478b7c0cb7afc6016fabdc400"}},"sourceBranch":"main","suggestedTargetBranches":["8.12"],"targetPullRequestStates":[{"branch":"8.12","label":"v8.12.1","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.13.0","labelRegex":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/173308","number":173308,"mergeCommit":{"message":"[Investigations]
- Unskip and refactor discover state tests (#173308)\n\nDependent on:
https://github.com/elastic/kibana/pull/173015\r\n\r\n- fixes
https://github.com/elastic/kibana/issues/165663\r\n- fixes
https://github.com/elastic/kibana/issues/165747\r\n\r\n[Flaky
Tests\r\n(100/100)](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4870)\r\n\r\n---------\r\n\r\nCo-authored-by:
Jan Monschke
<[email protected]>","sha":"1172c0ec09eb72f478b7c0cb7afc6016fabdc400"}}]}]
BACKPORT-->

---------

Co-authored-by: Michael Olorunnisola <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:cloud-deploy Create or update a Cloud deployment ci:project-persist-deployment Persist project deployment indefinitely release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.12.0 v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants