Skip to content

[App Search] use setSuccessMessage instead of flashSuccessToast for successful crawler domain deletes#107926

Closed
byronhulcher wants to merge 2 commits intoelastic:masterfrom
byronhulcher:forward-port-set-success-message
Closed

[App Search] use setSuccessMessage instead of flashSuccessToast for successful crawler domain deletes#107926
byronhulcher wants to merge 2 commits intoelastic:masterfrom
byronhulcher:forward-port-set-success-message

Conversation

@byronhulcher
Copy link
Copy Markdown
Contributor

Summary

Realized this today.

Checklist

Delete any items that are not applicable to this PR.

@byronhulcher byronhulcher requested a review from a team August 9, 2021 16:15
@byronhulcher byronhulcher added v7.15.0 auto-backport Deprecated - use backport:version if exact versions are needed backport:skip This PR does not require backporting Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes and removed backport:skip This PR does not require backporting labels Aug 9, 2021
@byronhulcher
Copy link
Copy Markdown
Contributor Author

Cheers @orhantoy!

@byronhulcher byronhulcher enabled auto-merge (squash) August 9, 2021 16:18
@cee-chen
Copy link
Copy Markdown
Contributor

cee-chen commented Aug 9, 2021

Just curious, why? All our other success messages in Kibana are now using toasts, so this seems like a UX inconsistency.

@byronhulcher byronhulcher disabled auto-merge August 9, 2021 17:35
@byronhulcher
Copy link
Copy Markdown
Contributor Author

byronhulcher commented Aug 9, 2021

@constancecchen so I was confused because in 7.x we use setSuccessMessage but in master we use flashSuccessToast. I thought I remembered us discussing that we were using too many toasts. I see now that when [App Search] Success toast polish pass was backported it did not include crawler_overview_logic.ts or crawler_overview_logic.test.ts in [7.x] [App Search] Success toast polish pass (#103410).

I will file a PR against 7.x to use flashSuccessToast universally

@byronhulcher
Copy link
Copy Markdown
Contributor Author

I've created #107948

@cee-chen
Copy link
Copy Markdown
Contributor

cee-chen commented Aug 9, 2021

Right yeah, crawler_overview_logic wasn't in 7.x at the time (but was in master) so I had to manually fix the merge conflict in the backport.

Honestly, I haven't been a huge fan of how the crawler backport situation played out with 7.x (a decent amount of conflicts/headaches) and going forward would generally suggest trying to keep master and 7.x 1:1 as possible and either utilizing dev environment checks or making manual "hide X feature" PRs to release branches only, like what I did for the entire plugin throughout 7.14 (#98197).

@cee-chen
Copy link
Copy Markdown
Contributor

cee-chen commented Aug 9, 2021

I remembered us discussing that we were using too many toasts

Also want to clarify on this - we (Enterprise Search) are not using too many toasts - we only currently use toasts for success messages. What we want to avoid is using toasts for error messages, which have potentially poorer UX with users missing messages before they disappear.

Other Kibana plugins/solutions use error toasts, we do not currently and still use flash messages callouts for 99% of our errors. The exception to this is transient polling errors, which will either recur every x seconds or fix itself on the next poll (if connection is restored) and thus do not have the same issue of being missed.

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

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

auto-backport Deprecated - use backport:version if exact versions are needed Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants