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.

Sign up for GitHub

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

Jump to bottom

[EuiPortal] convert to a function component, fix ssr bug #6055

Merged
merged 10 commits into from
Jul 27, 2022
Merged

[EuiPortal] convert to a function component, fix ssr bug #6055

merged 10 commits into from
Jul 27, 2022

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Jul 14, 2022 •
edited
Loading

Summary

Closes #5656

Converted EuiPortal to a function component, whose lifecycle now enforces SSR support.

I also deleted the euiBody-hasPortalContent class name instead of converting it to emotion. Wasn't used by EuiPortal, and the two usages (tooltips, combo boxes) don't appear to be affected by it (tested in Chrome, Safari, Edge, FF). I also searched Kibana where there are 5 mentions in issues (most of them working around the class), but nothing in code.

EuiBottomBar's tests had to be updated to use mount instead of render as the portal div is no longer available o the first render pass

Checklist

- [ ] Checked in both light and dark modes

  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
    - [ ] Props have proper autodocs and playground toggles
    - [ ] Added documentation
    - [ ] Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
    - [ ] Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@chandlerprall chandlerprall requested a review from cee-chen July 14, 2022 20:45
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6055/

@thompsongl thompsongl mentioned this pull request Jul 18, 2022
Closed
49 tasks
@thompsongl thompsongl self-requested a review July 19, 2022 14:11
@chandlerprall
Revert "remove euiBody-hasPortalContent css class and usages"
7b5835f 
This reverts commit c841fae.
@chandlerprall
Refactor EuiPortal to a function component; added unit test around po…
b6d6e9b 
…rtalRef; updated EuiBottomBar tests to account for new portal lifecycle
@chandlerprall
Update upcoming_changelogs/6055.md
12cacb6 
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@chandlerprall
Copy link
Contributor Author

@constancecchen @thompsongl I've rolled back the sass/emotion changes so this is purely changing class -> function component. Applied the applicable suggestions, and I added a test for portalRef usage.

@chandlerprall chandlerprall requested a review from cee-chen July 20, 2022 17:01
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6055/

@chandlerprall
Copy link
Contributor Author

@constancecchen insert logic works, and I've added four cypress tests to keep it that way

@chandlerprall chandlerprall requested a review from cee-chen July 22, 2022 19:32
cee-chen
cee-chen approved these changes Jul 22, 2022
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Very nice!!! Thanks for going above and beyond with the Cypress tests! Also digging the new data attribute for debugging (IMO, it'll be helpful even in the DOM/helping quickly scan for EUI portals).

Hopefully it won't generate a bunch of Kibana snapshot churn though hahaa nervous sweats

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6055/

@chandlerprall
Copy link
Contributor Author

Digging into the EuiPopover cypress failure

@thompsongl
Copy link
Contributor

thompsongl commented Jul 25, 2022 •
edited
Loading

@chandlerprall Try merging/rebasing main. One of the tests failing was recently removed (does not focus anything if initialFocus is false)

@chandlerprall
Copy link
Contributor Author

Cypress is happy after merging main; letting CI run again

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6055/

@chandlerprall
Copy link
Contributor Author

Passes locally, maybe a race condition; jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6055/

@chandlerprall
Copy link
Contributor Author

😕 a different test failed that time 😕

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6055/

@chandlerprall
Copy link
Contributor Author

The lifecycle change in the portal seems to have introduced an adverse race condition within the CI environment

@cee-chen
Copy link
Contributor

tour_step.spec.tsx - this one is legit and I think the expect/assertion needs to be rewritten. It's honestly not clear to me why it was previously passing.

I say this because it doesn't make sense as to why the <span id="anchor"> would exist inside the tour step div

@chandlerprall @thompsongl
Update upcoming_changelogs/6055.md
cf350c3 
Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>
@chandlerprall chandlerprall mentioned this pull request Jul 26, 2022
Merged
2 tasks
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6055/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6055/

@chandlerprall
Copy link
Contributor Author

The tour spec was failing as EuiWrappingPopover wasn't able to respond to the updated portal lifecycle. I've converted that component as well and now it's passing 🎆

cee-chen
cee-chen reviewed Jul 27, 2022
View reviewed changes
src/components/popover/wrapping_popover.tsx
Comment on lines +26 to +27
const [anchor, setAnchor] = useState<HTMLElement | null>(null);
const [portal, setPortal] = useState<HTMLElement | null>(null);
Copy link
Contributor

@cee-chen cee-chen Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should these be useRefs instead of state, based on the usage? edit to clarify: Not a change request, just curious what the reasoning is for state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They need to be state in order to kick-off a re-render after mount->setX calls, to trigger the useEffect. Assigning to a ref doesn't cause another pass.

@chandlerprall chandlerprall merged commit 3aacaf9 into elastic:main Jul 27, 2022
@chandlerprall chandlerprall deleted the bug/5656-euiportal-ssr branch July 27, 2022 16:10
@cee-chen cee-chen mentioned this pull request Jul 27, 2022
Closed
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6055/

@cee-chen cee-chen mentioned this pull request Aug 3, 2022
Merged
2 tasks
thompsongl pushed a commit to thompsongl/eui that referenced this pull request Aug 4, 2022
[EuiPortal] Revert function component change while maintaining SSR fix (
6091d38 
elastic#6105)

* Revert "[EuiPortal] convert to a function component, fix ssr bug (elastic#6055)"

This reverts commit 3aacaf9.

* Re-revert / keep portal test changes

* Restore `data-euiportal` attr for snapshot purposes

* Restore SSR bugfix + add SSR tests

* Changelog

* Skip failing Cypress test that doesn't apply to class component
@cee-chen cee-chen mentioned this pull request Aug 9, 2022
Merged
yuliacech pushed a commit to yuliacech/eui that referenced this pull request Aug 23, 2022
@cee-chen
[EuiPortal] Revert function component change while maintaining SSR fix (
6855472 
elastic#6105)

* Revert "[EuiPortal] convert to a function component, fix ssr bug (elastic#6055)"

This reverts commit 3aacaf9.

* Re-revert / keep portal test changes

* Restore `data-euiportal` attr for snapshot purposes

* Restore SSR bugfix + add SSR tests

* Changelog

* Skip failing Cypress test that doesn't apply to class component
@cee-chen cee-chen mentioned this pull request Jul 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen cee-chen approved these changes

@thompsongl thompsongl thompsongl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[EuiBottomBar] document is not defined when using server rendering
4 participants
@chandlerprall @kibanamachine @thompsongl @cee-chen