[EuiTable] Remove overflow/height animation causing Safari cross-browser issues#6826
Merged
cee-chen merged 5 commits intoelastic:mainfrom Jun 2, 2023
Merged
[EuiTable] Remove overflow/height animation causing Safari cross-browser issues#6826cee-chen merged 5 commits intoelastic:mainfrom
cee-chen merged 5 commits intoelastic:mainfrom
Conversation
The overflow/height combo causes Safari to return an empty `innerText` on initial render, which is problematic for expandable content that uses our inner text or copy utils. Since the animation honestly is adding nothing functional, and is slightly laggy, I opted for an immediate height expansion and a very slight opacity/translate Y animation for a little extra pizazz.
cee-chen
changed the title
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6826/ |
ryankeairns
requested changes
Jun 2, 2023
Contributor
ryankeairns
left a comment
ryankeairns
left a comment
There was a problem hiding this comment.
+1 for the overall simplicity.
Minor suggestion on the animation settings.
Contributor
Author
|
As a heads up, going to go ahead and revert the docs QA change since you've had a chance to test this already. |
This reverts commit c2cf575.
ryankeairns
approved these changes
Jun 2, 2023
Contributor
ryankeairns
left a comment
ryankeairns
left a comment
There was a problem hiding this comment.
Rad. Thanks for the change!
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6826/ |
cee-chen
added a commit
to elastic/kibana
that referenced
this pull request
Jun 3, 2023
This is a backport EUI upgrade to Kibana v8.8.1 containing an `EuiCodeBlock` bugfix requested by the Observability team: #158644 (comment) ## [`77.1.5`](https://github.com/elastic/eui/tree/v77.1.5) **Bug fixes** - Fixed `EuiCodeBlock` potentially incorrectly ignoring lines ending with a question mark when using the Copy button. ([#6794](elastic/eui#6794)) - Fixed `EuiCodeBlock` to not include line numbers when copying content ([#6824](elastic/eui#6824)) - Fixed the expanded row animation on `EuiBasicTable` causing cross-browser Safari issues ([#6826](elastic/eui#6826)) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
cee-chen
added a commit
to elastic/kibana
that referenced
this pull request
Jun 6, 2023
## Summary `eui@81.2.0` ⏩ `eui@81.3.0` ✨ Highlights: - fixes #158644 - Adds a new Timeline icon for use within `EuiMarkdownEditor` (cc @kqualters-elastic) - Expandable rows used within `EuiBasicTable` and `EuiInMemoryTable` now have a faster and snappier expand animation ___ ## [`81.3.0`](https://github.com/elastic/eui/tree/v81.3.0) - Added `timelineWithArrow` glyph to `EuiIcon` ([#6822](elastic/eui#6822)) **Bug fixes** - Fixed `EuiCodeBlock` potentially incorrectly ignoring lines ending with a question mark when using the Copy button. ([#6794](elastic/eui#6794)) - Fixed `EuiCodeBlock` to not include line numbers when copying content ([#6824](elastic/eui#6824)) - Fixed the expanded row animation on `EuiBasicTable` causing cross-browser Safari issues ([#6826](elastic/eui#6826))
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
closes #6770
Counter to our guesses/musings in the above linked thread, the issue with Safari was not the mutation observer (never fired) nor stale refs/rerenders. After adding timeouts of increasing lengths, I realized it was the animation itself that was causing Safari to return empty
innerText.The
overflow: hiddenon the container, in combination with themax-height, makes Safari calculate that the container's children are not visible. For not-visible children,innerTextbecomes''(you can test this right now in your browser by setting any DOM node tovisibility: hiddenand then pasting its.innerTextvalue in the console).I initially played around with a couple different options to try to keep the "expanding animation". I tried the
display: gridhack instead ofmax-height, I tried removingoverflowand set anopacityinstead (but didn't really like how it looked) and I briefly contemplated calculating and settingheightvia JS instead of CSS 😅 Finally, I decided I preferred to subtract instead of adding.Since the animation honestly is adding no actual functionality, and the
max-heightCSS hack is laggy on almost every browser, I opted to not animate height whatsoever which allows the expansion to be immediate, and instead added a very light and far more performant opacity/translate Y animation. IMO, the outcome is a snappier and slightly more responsive feel.QA
Old animation for reference: https://eui.elastic.co/v81.2.0/#/tabular-content/tables#expanding-rows
EuiCodeBlockin it (this may take several pages, it's random as the table is sorted)General checklist
- [ ] Added or updated jest and cypress tests- Not sure there's anything we can meaningfully test here since it's such a browser-specific issue + a CSS change- [ ] Updated the Figma library counterpart- Checking with the Figma working group to see if animations are a part of our Figma library