-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Upgrade EUI to v31.7.0 #91210
Upgrade EUI to v31.7.0 #91210
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data grid is adding a light shade of grey as background now:
This is very distracting during virtualized scrolling because the grey flickers before it's replaced with new cells. What would be the best way to get the background back to white? The .euiDataGrid__content
element is the issue here so we can't overwrite it from the outside without reaching into the EUI element structure which I would like to avoid if possible (if it's not, we can simply do that, it's not a big issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alerting changes LGTM
@azasypkin Can you provide more environment info? I cannot reproduce the issue you're seeing on this branch: |
Hmm, let me double check that I didn't mess up with anything. |
Deferring to @chandlerprall and @snide for what to do here. What do you two think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flash1293 @thompsongl it is intentional, see https://github.com/elastic/eui/pull/4509/files#diff-5826316bba7a42b0d9228a47cdc8c8e67ed3eee9d092982cd5c4292258b7bc4fR36 |
The background color was intentional and arrived at after feedback in elastic/eui#4170 (review). I wouldn't hold up this PR over it since it's a minor styling issue that can be changed with a one-liner on either side of the code. Likely we've got some back and forth with subjective opinions till everyone is happy, and that won't happen before tomorrow. Ultimately, the heart of the problem is that I think we need a better concept of loading state with cells that are virtualized. That's definitely harder and will take another minor to land. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! No idea what was wrong with my environment, but after yarn kbn clean
and re-bootstrapping everything seems to be in order, sorry for the noise.
Actyally it worked after running a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fleet changes 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
infra
plugin changes LGTM 👍
(had to clean
as well to get it working, though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the datagrid coloring got discussed and is the outcome of a deliberate decision, I'm happy with the current status, we can iterate on this.
What would be cool is to use a repeating background image of the cell borders so it's just "filling in" the content during scrolling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ML changes LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code only review, everything makes sense. Presentation Team changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge thanks from the App Search team!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ES UI changes LGTM. Left one nit regarding removing use of fragment.
...tion/public/app/components/follower_index_actions_providers/follower_index_pause_provider.js
Outdated
Show resolved
Hide resolved
...ugins/cross_cluster_replication/public/app/components/auto_follow_pattern_delete_provider.js
Outdated
Show resolved
Hide resolved
...ion/public/app/components/follower_index_actions_providers/follower_index_resume_provider.js
Outdated
Show resolved
Hide resolved
...n/public/app/components/follower_index_actions_providers/follower_index_unfollow_provider.js
Outdated
Show resolved
Hide resolved
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
Unknown metric groups@kbn/ui-shared-deps asset size
History
To update your PR or re-run it, just comment with: |
* eui to 31.6.0 * flyout, collapsible snapshot updates * initial overlaymask removal * undo jest * overlaymask src snapshot updates * more overlaymask removals * overlaymask removal xpack test updates * saved objects modal form * eui to 31.7.0 * code, codeblock types * snapshot update * tooltip * remove ownFocus from ConfirmModal * remove fragments # Conflicts: # x-pack/plugins/canvas/public/components/asset_manager/asset_manager.component.tsx
* eui to 31.6.0 * flyout, collapsible snapshot updates * initial overlaymask removal * undo jest * overlaymask src snapshot updates * more overlaymask removals * overlaymask removal xpack test updates * saved objects modal form * eui to 31.7.0 * code, codeblock types * snapshot update * tooltip * remove ownFocus from ConfirmModal * remove fragments # Conflicts: # x-pack/plugins/canvas/public/components/asset_manager/asset_manager.component.tsx
eui@31.4.0
⏩eui@31.7.0
EuiOverlayMask
is now included inEuiModal
andEuiConfirmModal
31.7.0
whiteSpace
prop toEuiCodeBlock
. (#4475)EuiDataGrid
and removed unnecessary height on its container (#4509)Bug fixes
EuiDataGrid
where the grid lost height when showing less rows on the last page (#4509)euiPaletteForStatus
color sequence to use higher contrast postive and negative colors. (#4508)31.6.0
axe-puppeteer v1.1.1
to@axe-core/puppeteer v4.1.1
(#4482)EuiOverlayMask
directly toEuiModal
(#4480)paddingSize
prop toEuiFlyout
(#4448)size='l'
prop toEuiTabs
(#4501)pageTitle
,description
,tabs
,rightSideItems
) toEuiPageHeader
by creating a newEuiPageHeaderContent
component (#4451)isActive
parameter to theuseIsWithinBreakpoints
hook (#4451)buttonProps
prop toEuiAccordion
(#4510)Bug fixes
onClose
invoking with unexpected parameter inEuiFlyout
(#4505)EuiBadge
color prop (#4481)EuiCodeBlock
focus-state if content overflows #4463EuiDataGrid
around unnecessary scroll bars and container heights not updating (#4468)Theme: Amsterdam
EuiPage
's defaultrestrictWidth
size to1200px
(extra large breakpoint) (#4451)euiBottomShadowSmall
by one layer (#4451)31.5.0
isLoading
prop and addedEuiOverlayMask
directly toEuiConfirmModal
(#4421)wrapWithTableRow
to remove<tr>
inEuiTableHeader
(#4465)Bug fixes
id
usage throughoutEuiTreeView
to respect custom ids and stop conflicts in generated ids (#4435)EuiTabs
role
if no tabs are passed in (#4435)EuiDataGrid
where the horizontal scrollbar was hidden behind pagination (#4477)EuiPopover
with initialisOpen
working withEuiOutsideClickDetector
(#4461)EuiDataGridCellPopover
needing 2 state updates to close (#4461)EuiBadge
withiconOnClick
from catching form submit events (#4479)