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

[drift] Update Drift Storybook to apply context, fix bug #139510

Merged
merged 5 commits into from
Aug 30, 2022

Conversation

clintandrewhall
Copy link
Contributor

@clintandrewhall clintandrewhall commented Aug 25, 2022

Summary

The Storybook example for Drift chat was lacking the context provider, which in turn provides parameters to Drift chat. This PR fixes that issue, and adds argument fields to configure Drift. It also fixes a bug where Drift may not appear.

Screen Shot 2022-08-29 at 10 41 10 AM

The bug

While working on this, I discovered a bug/change that sometimes prevents Drift from being displayed completely. When I wrote this (in #123772), Drift would "bounce around" unless we waited for the driftIframeResize event to fire. At some point between that PR and now, driftIframeResize doesn't always fire, so the chat window doesn't become visible.

The fix

  • Set the initial height of the iframe to 0, rather than use visibility to maintain its state.
  • Use visibility to maintain the state of the 'Hide chat` button, and only that button.

Testing Notes

You have to be sure there's a localhost enabled playbook for Storybook to work.

@clintandrewhall clintandrewhall added review Team:Cloud loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. auto-backport Deprecated - use backport:version if exact versions are needed v8.5.0 v8.4.2 labels Aug 25, 2022
@clintandrewhall clintandrewhall requested a review from a team as a code owner August 25, 2022 19:18
@claracruz
Copy link
Contributor

So running on this branch, looks like this resolves the issue with storybook for me, at least on chrome.

Firefox fails but that seems to be due to this, still investigating. Safari is still failing and drift-iframe.html requested twice now not 3 - 4 times as before.

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Did not run this locally, but code changes LGTM

@lukeelmers
Copy link
Member

@elasticmachine merge upstream

@clintandrewhall clintandrewhall changed the title [drift] Update Drift Storybook to apply context [drift] Update Drift Storybook to apply context, fix bug Aug 29, 2022
@clintandrewhall
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 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
cloud 7.6KB 7.7KB +82.0B

History

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

Copy link
Contributor

@claracruz claracruz left a comment

Choose a reason for hiding this comment

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

Pulled and tested, works in Chrome, Firefox and Safari 🎉

@clintandrewhall clintandrewhall merged commit af42246 into elastic:main Aug 30, 2022
kibanamachine pushed a commit that referenced this pull request Aug 30, 2022
* [drift] Update Drift Storybook to apply context

* A few tweaks

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit af42246)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.4

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

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

kibanamachine added a commit that referenced this pull request Aug 30, 2022
…139769)

* [drift] Update Drift Storybook to apply context

* A few tweaks

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit af42246)

Co-authored-by: Clint Andrew Hall <clint.hall@elastic.co>
@clintandrewhall clintandrewhall self-assigned this Sep 1, 2022
Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022
)

* [drift] Update Drift Storybook to apply context

* A few tweaks

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
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 impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:Cloud v8.4.1 v8.4.2 v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants