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

[Search Session] Revamp search session indicator UI and tour. #89703

Merged
merged 31 commits into from
Feb 4, 2021

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Jan 29, 2021

Summary

Part of #61738

Doing a small search session indicator UI closer to recent figma + adding a Tour like functionality when can auto-opens in the first run.

  • This pr also removes the "refresh" button from the canceled/completed/restored state because it added complexity and was doing the same thing as "refresh" from the query bar.

Please note, out of scope in this PR comparing to Figma:

  • x Session name display
  • x Session name editing
  • x Dates/timestamps display

Screenshot 2021-02-02 at 15 07 19
Screenshot 2021-02-02 at 15 07 23
Screenshot 2021-02-02 at 16 55 03
Screenshot 2021-02-02 at 15 08 36
Screenshot 2021-02-02 at 15 08 55
Screenshot 2021-02-02 at 15 09 02

To enable feature in kibana and to kibana.yml:

xpack.data_enhanced.search.sessions.enabled: true

To play in the storybook:

yarn storybook data_enhanced

Tour

As per figma we agreed instead of a separate tour just-auto open the popover in the following cases:

  • In loading state search take longer than 10 seconds
  • first time when navigated to restored state

Popover is auto-opened once and info about whether it was opened is persisted in local storage.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

because removed debounce when switching to NONE state
@Dosant Dosant added Feature:Search Querying infrastructure in Kibana Team:AppServices v7.12.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jan 29, 2021
@Dosant Dosant marked this pull request as ready for review January 29, 2021 21:11
@Dosant Dosant requested review from a team as code owners January 29, 2021 21:11
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@Dosant Dosant requested review from mdefazio, gchaps and lizozom January 29, 2021 21:11
@Dosant
Copy link
Contributor Author

Dosant commented Feb 1, 2021

@elasticmachine merge upstream

@Dosant Dosant requested a review from a team February 1, 2021 14:54
@Dosant Dosant requested a review from a team as a code owner February 1, 2021 14:54
@Dosant
Copy link
Contributor Author

Dosant commented Feb 1, 2021

@elasticmachine merge upstream

@mdefazio
Copy link
Contributor

mdefazio commented Feb 1, 2021

After discussing with @gchaps , here is where we ended up with the copy.

Data loading New icon:
image

Your search is taking a while...
Save your session, continue your work, and return to completed results.

Data complete:
image

Search session complete
Save your session and return to it later.

Session saved that is in progress:
image

Saved session in progress. 
You can return to completed results from Management.

Session saved that was complete:
image

Search session saved
You can return to these results from Management.

Saved Session complete:
image

Saved session complete. 
You can return to these results from Management.

Session restored New icon:
image

Saved session restored
You are viewing cached data from a specific time range. Changing the time range or filters will re-run the session.```

Session canceled:
image

Search session stopped
You are viewing incomplete data.

I am introducing 2 new icons that are slight variants to current icons. My hope is that these help introduce these new concepts, but aren't using icons that are potentially known for other things in Kibana. In regards to the loading / clock icon, this will make more sense when we introduce the time stamp. However, I think they still work without it. I know I'm suggesting a departure from the restored state icon, but I still think it's important to have that continuity from management with a green checkmark, so the dashboard with a green checkmark. This is why I did a slight variation on this to suggest the subtle difference between this view, compared to the view if I stay on the page and it completes.


If we think the restored state does not provide enough indication that the time range is set to absolute, I would suggest using the tour component for this and highlighting the date picker. Example below: This is not final copy

image

}: SearchSessionIndicatorDeps): React.FC => {
const isAutoRefreshEnabled = () => !timeFilter.getRefreshInterval().pause;
const isAutoRefreshEnabled$ = timeFilter
.getRefreshIntervalUpdate$()
.pipe(map(isAutoRefreshEnabled), distinctUntilChanged());

const debouncedSessionServiceState$ = sessionService.state$.pipe(
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't you listen to the appId$ change instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, for example: navigating from a dashboard page to a dashboard listing page

@Dosant
Copy link
Contributor Author

Dosant commented Feb 2, 2021

@mdefazio,

I think it might also make sense to use 'Search session saved' as opposed to 'Saved session complete' if we have to merge these 2 states.

Done!

Testing this in dark mode and in Amsterdam: It looks like because the session indicator icon is in the breadcrumbs, it's breaking the height of the breadcrumbs. Also, due to the color of the current page, it makes the icon nearly unreadable.

Created an issue, will prioritize and work in separate pr: #90028
Since this isn't introduced in this pr but is also in master, I wouldn't block this pr.

@Dosant Dosant requested a review from mdefazio February 2, 2021 16:02
@Dosant
Copy link
Contributor Author

Dosant commented Feb 2, 2021

@elasticmachine merge upstream

@mdefazio
Copy link
Contributor

mdefazio commented Feb 2, 2021

@gchaps and I were trying to finalize the copy for the tour component, but we had a few questions.

image

  1. Is a search session will be accessible to anyone in that space? So the person viewing this (for the first time) might not be the person who saved the session?
  2. Will we know if the time range was previously relative (but is now absolute)? And so will this show regardless if it was relative or absolute?
  3. Is the primary purpose of this to forewarn them about changing filters/time since it will likely be a long-running session? Or is it about changing from relative time to absolute time?

Gail, let me know if I missed any of our questions.

And let me know if this is not the right issue for this discussion.

@Dosant
Copy link
Contributor Author

Dosant commented Feb 3, 2021

@mdefazio, I created an issue for the time range change tour: #90130, let's continue the discussion there

…i-revamp

# Conflicts:
#	x-pack/test/send_search_to_background_integration/services/search_sessions.ts
#	x-pack/test/send_search_to_background_integration/tests/apps/management/search_sessions/sessions_management.ts
Copy link
Member

@dmlemeshko dmlemeshko left a comment

Choose a reason for hiding this comment

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

kibana-qa changes LGTM

@Dosant
Copy link
Contributor Author

Dosant commented Feb 3, 2021

@elasticmachine merge upstream

Copy link
Contributor

@mdefazio mdefazio 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! Thanks for working through all the last minute design edits!

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Tiny Dashboard change LGTM. Code only review.

The new designs are looking awesome, can't wait to see them in master!

@kertal
Copy link
Member

kertal commented Feb 3, 2021

ACK, code LGTM, will test tomorrow morning

@kertal
Copy link
Member

kertal commented Feb 4, 2021

@elasticmachine merge upstream

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.

KibanaApp owned code LGTM, tested locally in Chrome, works and looks as beautiful as expected 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dataEnhanced 93 95 +2

Async chunks

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

id before after diff
dashboard 176.4KB 176.4KB -31.0B
dataEnhanced 150.6KB 147.7KB -2.9KB
discover 417.1KB 416.9KB -159.0B
total -3.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 797.9KB 797.7KB -202.0B
dataEnhanced 33.6KB 36.1KB +2.6KB
total +2.4KB

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants