-
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
Bump eui #20774
Bump eui #20774
Conversation
💔 Build Failed |
@stacey-gammon what version(s) are you planning to target here? |
@w33ble, 6.x and 7.0. I'm not sure of the protocol, but I have some typings that are in 2.0 that I'd like to work against. I also added a comment on your PR. |
Mainly adding `EuiPageBody`’s where there were none
💔 Build Failed |
Fixed breaking `EuiPage` changes
💔 Build Failed |
3f06ba5
to
bc453c0
Compare
💚 Build Succeeded |
@stacey-gammon I'm confused, where did my fixes PR go? The changes are not in here and the homepage layout is broke again. |
that's really weird @cchaos! I have no idea. Hmmm, let me see if I can remerge the changes. |
💔 Build Failed |
💔 Build Failed |
well I added your changes and updated the xpack snapshot tests @cchaos but i think now fialing on yet more flaky tests 😭
lets find out. jenkins, test this. |
Yeah, something about this missing selector |
💚 Build Succeeded |
Filed the flaky test issue here: #20893 ready for review |
Ready for final review. Am out tomorrow - Friday so if I don't get it in today, this will either have to wait till next week or someone else will have to take over. |
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.
I believe all the EuiPage
instances have been found and updated with EuiPageBody
. LGTM
@cchaos - is this supposed to happen on dashboard listing page (note vertically centered is new i think): Tried to manually review all the other changed snapshots and they look good as far as i can tell. |
@stacey-gammon Yes, that's on purpose via this line introduced during the EUI-ification of that page. |
Ha. I'll clean that up post this PR @stacey-gammon. That line @cchaos is pointing to likely wasn't working (she fixed it!) when I implemented it. I agree it looks goofy :) Thanks for getting this stuff in. |
<EuiLink href="#/home">Home</EuiLink> / <EuiLink href="#/home/tutorial_directory">Add Data</EuiLink> | ||
<EuiSpacer size="s" /> | ||
{content} | ||
<div> |
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.
Why is the EUILink now wrapped in a div
?
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.
@cchaos can you answer this? It's part of the code you made that I merged into this PR.
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.
Because EuiPageBody
is a flex-box of column direction and so each child would be on it's on line even if they're inline style elements like link tags. Wrapping these breadcrumbs in a div, keeps them as one individual flex-item.
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.
sorry for the double-post, gh is being stupid
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 other than a question about adding a div element
code review, looked through home page, add data, dashboard, and management screens to view new EUI look
@stacey-gammon Looks like this should be gtg! |
Great! I'm on my phone right now, ended with for the day. Will merge once
I get to a computer or feel free to hit the merge button sooner if it's
blocking anyone. Not logged in on my phone so can't do it this second.
…On Tue, Jul 17, 2018, 6:23 PM Caroline Horn ***@***.***> wrote:
@stacey-gammon <https://github.com/Stacey-Gammon> Looks like this should
be gtg!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20774 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/APy9kx91FOIJvR_NDjM7Js3_JVj1pZcMks5uHmP1gaJpZM4VPInZ>
.
|
* bump eui * Fixed breaking `EuiPage` changes Mainly adding `EuiPageBody`’s where there were none * bump to 3.0, remove duplicate declaration of EuiFlyoutBody, update jest snapshots * bump eui * bump to 3.0, remove duplicate declaration of EuiFlyoutBody, update jest snapshots * Update jest snapshots in xpack
* bump eui * Fixed breaking `EuiPage` changes Mainly adding `EuiPageBody`’s where there were none * bump to 3.0, remove duplicate declaration of EuiFlyoutBody, update jest snapshots * bump eui * bump to 3.0, remove duplicate declaration of EuiFlyoutBody, update jest snapshots * Update jest snapshots in xpack
For the future, reminder to everyone that we need to set dependencies is BOTH package.json and xpack/package.json. No worries, I missed it to. Have a follow up PR to fix. #20930 |
Lets just take the dependency out of x-pack. That way there is only one version |
No description provided.