-
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 Assistant] Migrate all usages of EuiPage*_Deprecated #163127
Conversation
Co-authored-by: LuisChiej <[email protected]> Co-authored-by: gitstart_bot <[email protected]>
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
Hi @gitstart! Thank you for your contribution ❤️ Can you please provide screenshots of the changes you have made? This will help us greatly with the review and confirm the changes have been tested. Also - a quick note: While the “How” section in #161413 provides a quick conversion map, unfortunately, this does not work for all cases. Please refer to the EUI guidelines on how to approach the migration and examples. You can also take a look at this PR as a reference implementation: #163134. Please let us know if you have any questions. Thanks! |
Yes, a quick question. How can we run it and find exactly the visual changes made |
Co-authored-by: LuisChiej <[email protected]> Co-authored-by: gitstart_bot <[email protected]>
@gitstart Please refer to the dev documentation on how to run kibana locally. In short that would be the following:
|
Hello @yuliacech, Thank you for this. We already run this, I am though referring to how to trace the components easily to take screenshots. Thank you 😊 |
|
You can view Upgrade Assistant at |
Co-authored-by: LuisChiej <[email protected]> Co-authored-by: gitstart_bot <[email protected]>
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
@elasticmachine merge upstream |
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.
Thanks for your contribution @gitstart! I'd like to see one more change made in deprecations_page_loading_error.tsx
, otherwise code looks good.
...s/upgrade_assistant/public/application/components/shared/deprecations_page_loading_error.tsx
Show resolved
Hide resolved
buildkite test this |
@gitstart please also review the CI failures in #163127 (comment) |
Co-authored-by: LuisChiej <[email protected]> Co-authored-by: gitstart_bot <[email protected]>
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.
Hi @gitstart - we're getting close. It looks like some of the data-test-subj
data attributes were lost in the changes, which is causing tests to fail. I tried to identify the places that need to be addressed in the code. Please have a look. You can see more information about the test failures in #163127 (comment).
/> | ||
</p> | ||
} | ||
data-test-subj="emptyPrompt" |
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.
We should preserve the same top-level data-test-subj
so no changes are required in the test
data-test-subj="emptyPrompt" | |
data-test-subj="isUpgradeCompleteMessage" |
/> | ||
</p> | ||
} | ||
data-test-subj="emptyPrompt" |
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.
We should preserve the same top-level data-test-subj
so no changes are required in the test
data-test-subj="emptyPrompt" | |
data-test-subj="isUpgradingMessage" |
color="danger" | ||
data-test-subj="deprecationsPageLoadingError" |
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.
We need to retain this data-test-subj
for tests
Co-authored-by: LuisChiej <[email protected]> Co-authored-by: gitstart_bot <[email protected]>
@elasticmachine merge upstream |
buildkite test this |
@elasticmachine run elasticsearch-ci/docs |
@elasticmachine merge upstream |
@elasticmachine run elasticsearch-ci/docs |
buildkite test this |
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
What does this PR do?
Issue References
Fixes #163071
Video/Screenshot Demo
Path
x-pack/plugins/upgrade_assistant/public/application/components/es_deprecation_logs/es_deprecation_logs.tsx
Path:
x-pack/plugins/upgrade_assistant/public/application/app.tsx
Not authorized:
Authorized:
Is cluster upgrading:
Cluster is upgraded:
This code was written and reviewed by GitStart Community. Growing great engineers, one PR at a time.