Fix EuiSideBar with css props not being detected by EuiPageTemplate
#6324
Conversation
This file contains 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
…it to render incorrectly
cee-chen
commented
Oct 25, 2022
Comment on lines
+162
to
+165
| if ( | ||
| child.type === EuiPageSidebar || | ||
| child.props.__EMOTION_TYPE_PLEASE_DO_NOT_USE__ === EuiPageSidebar | ||
| ) { |
There was a problem hiding this comment.
Just to clarify a few things:
- Is this the most robust solution? Absolutely not.
- Is this a pretty okay but kinda janky solution to unblock Kibana that will cover 95% of use cases? Yep!
This PR really only fixes Emotion usage and not any other potential wrappers/HOCs/etc around EuiPageSidebar. However, I'm personally okay with this, at least for now. If we get more reports down the road or more wrapper use-cases, we can re-evaluate a more robust solution - e.g. recursing into grandchildren to check for type - but I'm loathe to add that level of over-engineering at this stage.
I also feel confident that the unit test I've included (which checks for presence of items within main) will catch any regressions, should Emotion's internals change.
There was a problem hiding this comment.
At least this one only asks nicely that you don't use it, unlike React's variable that would fire you
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6324/ |
chandlerprall
approved these changes
Oct 25, 2022
|
I think we should try to hold off on backporting this into the existing update. Let's see how the rest of the week progresses with the update; I can definitely see a backport with this and a couple other items that are important for the upcoming stack FF, but would prefer to keep them isolated from the current hurdles. |
|
👍 I think I have my FF dates all messed up. As long as the fix gets in before 8.6, all good! |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6324/ |
3 tasks
8 tasks
jbudz
pushed a commit
to elastic/kibana
that referenced
this pull request
Nov 18, 2022
## Summary `eui@67.1.8` ⏩ `eui@70.2.2`⚠️ Note: This upgrade contains breaking changes to `EuiFlexGroup` and `EuiFlexGrid`, primarily around switching margins and negative margins to `gap`. Please do a quick QA pass of your app to scan for any issues. We're happy to help resolve minor fixes, or potentially follow up after PR merges. You can find us over in #eui! ## [`70.2.4`](https://github.com/elastic/eui/tree/v70.2.4) **Bug fixes** - Fixed visual bug in nested `EuiFlexGroup`s, where the parent `EuiFlexGroup` is responsive but a child `EuiFlexGroup` is not ([#6381](elastic/eui#6381)) ## [`70.2.3`](https://github.com/elastic/eui/tree/v70.2.3) **Bug fixes** - Fixed incorrect margins in `EuiSuperDatePicker` caused by `EuiFlex` CSS gap change ([#6380](elastic/eui#6380)) ## [`70.2.2`](https://github.com/elastic/eui/tree/v70.2.2) - `EuiButton` now accepts `minWidth={false}` ([#6373](elastic/eui#6373)) **Bug fixes** - `EuiButton` no longer outputs unnecessary inline styles for `minWidth={0}` or `minWidth={false}` ([#6373](elastic/eui#6373)) - `EuiFacetButton` no longer reports type issues when passing props accepted by `EuiButton` ([#6373](elastic/eui#6373)) - Fixed the shadow sizes of `.eui-yScrollWithShadows` and `.eui-xScrollWithShadows` ([#6374](elastic/eui#6374)) ## [`70.2.1`](https://github.com/elastic/eui/tree/v70.2.1) **Bug fixes** - Re-fixed `EuiPageSection` not correctly merging `contentProps.css` ([#6365](elastic/eui#6365)) - Fixed `EuiTab` not defaulting to size `m` ([#6366](elastic/eui#6366)) ## [`70.2.0`](https://github.com/elastic/eui/tree/v70.2.0) - Added a keyboard shortcuts popover to `EuiDataGrid`'s toolbar. This can be visually hidden via `toolbarVisibility.showKeyboardShortcuts`, but will always remain accessible to keyboard and screen reader users. ([#6036](elastic/eui#6036)) - `EuiScreenReaderOnly`'s `showOnFocus` prop now also shows on focus within its children ([#6036](elastic/eui#6036)) - Added `onFocus` prop callback to `EuiSuperDatePicker` ([#6320](elastic/eui#6320)) **Bug fixes** - Fixed `EuiSelectable` to ensure the full options list is re-displayed when the search bar is controlled and cleared using `searchProps.value` ([#6317](elastic/eui#6317)) - Fixed incorrect padding on `xl`-sized `EuiTabs` ([#6336](elastic/eui#6336)) - Fixed `EuiCard` not correctly merging `css` on its child `icon`s ([#6341](elastic/eui#6341)) - Fixed `EuiCheckableCard` not setting `css` on the correct DOM node ([#6341](elastic/eui#6341)) - Fixed a webkit rendering issue with `EuiModal`s containing `EuiBasicTable`s tall enough to scroll ([#6343](elastic/eui#6343)) - Fixed bug in `to_initials` that truncates custom initials ([#6346](elastic/eui#6346)) - Fix bug in `EuiCard` where layout breaks when `horizontal` and `selectable` are both passed ([#6348](elastic/eui#6348)) ## [`70.1.0`](https://github.com/elastic/eui/tree/v70.1.0) - Added the `hint` prop to the `<EuiSearchBar />`. This prop lets the consumer render a hint below the search bar that will be displayed on focus. ([#6319](elastic/eui#6319)) - Added the `hasDragDrop` prop to `EuiPopover`. Use this prop if your popover contains `EuiDragDropContext`. ([#6329](elastic/eui#6329)) **Bug fixes** - Fixed `EuiButton`'s cursor style when the button is disabled ([#6323](elastic/eui#6323)) - Fixed `EuiPageTemplate` not recognizing child `EuiPageSidebar`s/`EuiPageTemplate.Sidebar`s with `css` props ([#6324](elastic/eui#6324)) - Fixed `EuiBetaBadge` to always respect its `anchorProps` values, including when there is no tooltip content ([#6326](elastic/eui#6326)) - Temporarily patched `EuiModal` to not cause scroll-jumping issues on modal open ([#6327](elastic/eui#6327)) - Fixed buggy drag & drop behavior within `EuiDataGrid`'s columns & sorting toolbar popovers ([#6329](elastic/eui#6329)) - Fixed `EuiButton` not correctly passing `textProps` for children inside fragments or i18n components ([#6332](elastic/eui#6332)) - Fixed `EuiButton` not correctly respecting `minWidth={0}` ([#6332](elastic/eui#6332)) **CSS-in-JS conversions** - Converted `EuiTabs` to Emotion ([#6311](elastic/eui#6311)) ## [`70.0.0`](https://github.com/elastic/eui/tree/v70.0.0) - Added the `enabled` option to the `<EuiInMemoryTable />` `executeQueryOptions` prop. This option prevents the Query from being executed when controlled by the consumer. ([#6284](elastic/eui#6284)) **Bug fixes** - Fixed `EuiOverlayMask` to set a `[data-relative-to-header=above|below]` attribute to replace the `--aboveHeader` and `--belowHeader` classNames removed in its Emotion conversion ([#6289](elastic/eui#6289)) - Fixed `EuiHeader` CSS using removed `EuiOverlayMask` class modifiers ([#6293](elastic/eui#6293)) - Fixed `EuiToolTip` not respecting reduced motion preferences ([#6295](elastic/eui#6295)) - Fixed a bug with `EuiTour` where passing any `panelProps` would cause the beacon to disappear ([#6298](elastic/eui#6298)) **Breaking changes** - `@emotion/css` is now a required peer dependency, alongside `@emotion/react` ([#6288](elastic/eui#6288)) - `@emotion/cache` is no longer required peer dependency, although your project must still use it if setting custom cache/injection locations ([#6288](elastic/eui#6288)) **CSS-in-JS conversions** - Converted `EuiCode` and `EuiCodeBlock` to Emotion; Removed `euiCodeSyntaxTokens` Sass mixin and `$euiCodeBlockPaddingModifiers`; ([#6263](elastic/eui#6263)) - Converted `EuiResizableContainer` and `EuiResizablePanel` to Emotion ([#6287](elastic/eui#6287)) ## [`69.0.0`](https://github.com/elastic/eui/tree/v69.0.0) - Added support for `fullWidth` prop on EuiForm, which will be the default for all rows/controls within ([#6229](elastic/eui#6229)) - Added support for `onResizeStart` and `onResizeEnd` callbacks to `EuiResizableContainer` ([#6236](elastic/eui#6236)) - Added optional case sensitive option matching to `EuiComboBox` with the `isCaseSensitive` prop ([#6268](elastic/eui#6268)) - `EuiFlexItem` now supports `grow={0}` ([#6270](elastic/eui#6270)) - Added the `alignItems` prop to `EuiFlexGrid` ([#6281](elastic/eui#6281)) - Added `filter`, `filterExclude`, `filterIgnore`, `filterInclude`, `indexTemporary`, `infinity`, `sortAscending`, and `sortDescending` glyphs to `EuiIcon` ([#6282](elastic/eui#6282)) **Bug fixes** - Fixed `EuiTextProps` to show the `color` type option `inherit` as default ([#6267](elastic/eui#6267)) - `EuiFlexGroup` now correctly respects `gutterSize` when responsive ([#6270](elastic/eui#6270)) - Fixed the last breadcrumb in `EuiBreadcrumbs`'s `breadcrumbs` array not respecting `truncate` overrides ([#6280](elastic/eui#6280)) **Breaking changes** - `EuiFlexGrid` no longer supports `columns={0}`. Use `EuiFlexGroup` instead for normal flex display ([#6270](elastic/eui#6270)) - `EuiFlexGrid` now uses modern `display: grid` CSS ([#6270](elastic/eui#6270)) - `EuiFlexGroup`, `EuiFlexGrid`, and `EuiFlexItem` now use modern `gap` CSS instead of margins and negative margins ([#6270](elastic/eui#6270)) - `EuiFlexGroup` no longer applies responsive styles to `column` or `columnReverse` directions ([#6270](elastic/eui#6270)) **CSS-in-JS conversions** - Converted `EuiFlexGroup`, `EuiFlexGrid`, and `EuiFlexItem` to Emotion ([#6270](elastic/eui#6270)) ## [`68.0.0`](https://github.com/elastic/eui/tree/v68.0.0) - Added `beta` glyph to `EuiIcon` ([#6250](elastic/eui#6250)) - Added `launch` and `spaces` glyphs to `EuiIcon` ([#6260](elastic/eui#6260)) - Added the `fallbackDestination` prop to `EuiSkipLink`, which accepts a string of query selectors to fall back to if the `destinationId` does not have a valid target. Defaults to `main` ([#6261](elastic/eui#6261)) - `EuiSkipLink` is now always an `a` tag to ensure that it is always placed within screen reader link menus. ([#6261](elastic/eui#6261)) **Bug fixes** - Fixed `EuiSuperDatePicker` not correctly merging passed `className`s ([#6253](elastic/eui#6253)) - Fixed `EuiColorStops` not correctly merging in passed `data-test-subj`s, `style`s, or `...rest` ([#6255](elastic/eui#6255)) - Fixed `EuiResizablePanel` incorrectly passing `style` to the wrapper instead of the panel. Use `wrapperProps.style` to pass styles to the wrapper. ([#6255](elastic/eui#6255)) - Fixed custom `onClick`s passed to `EuiSkipLink` overriding `overrideLinkBehavior` ([#6261](elastic/eui#6261)) **Breaking changes** - Removed `inherit` and `ghost` color from `EuiListGroupItem` ([#6207](elastic/eui#6207)) - Changed default color to `text` instead of `inherit` ([#6207](elastic/eui#6207)) **CSS-in-JS conversions** - Converted `EuiListGroup` and `EuiListGroupItem` to Emotion; Removed `$euiListGroupGutterTypes`, `$euiListGroupItemColorTypes` and `$euiListGroupItemSizeTypes`; ([#6207](elastic/eui#6207)) - Converted `EuiBadgeGroup` to Emotion ([#6258](elastic/eui#6258)) - Converted `EuiBetaBadge` to Emotion ([#6258](elastic/eui#6258)) - Converted `EuiNotificationBadge` to Emotion ([#6258](elastic/eui#6258)) Co-authored-by: Elizabet Oliveira <elizabet.oliveira@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
edmarmoretti
pushed a commit
to edmarmoretti/eui
that referenced
this pull request
May 3, 2023
…elastic#6324) * Fix issue where using a `css` prop on EuiPageTemplate.Sidebar causes it to render incorrectly * Add missing children unit tests * changelog * [test] Confirm that non-component JSX does not cause obj errors
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
See elastic/kibana#143114 (comment) for more context. This PR will likely need to be backported to the latest Kibana upgrade to resolve the linked issue.
When the
cssprop is set on a component, Emotion wraps it, changing.typeEuiPageTemplate is using to detect sidebars. By expanding the conditions for checking for EuiPageSidebar, we can work around this behavior to fix the bug.QA
General 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- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Updated the Figma library counterpart