-
Notifications
You must be signed in to change notification settings - Fork 72
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
Remove charts theme code #554
Remove charts theme code #554
Conversation
Thanks for the contribution! There are a few things that I would like to be completed before we review this PR. Specifically, could the description be filled out, specifically the Also, it looks like DCO didn't pass. Do you think you could sign your commit? Related links: |
Hello @tarunsamanta2k20, in order to merge this PR, you need to do the following steps:
You can copy these commands and run them:
After that, you can select the FYI: @tarunsamanta2k20, please don't forget to remove Thanks for contributing! |
Hello @tarunsamanta2k20, I see that you changed the description of the issue! Well done! Now just need to pass the DCO pipeline. Please try to run these commands:
And don't forget to push this updated commit to your branch:
Just a little bit left! Thanks for your changes! |
Signed-off-by: Tarun Samanta <[email protected]>
@BSFishy done! |
@tarunsamanta2k20 The linter check is failing because there's still a reference to one of the deleted files that needs to be removed: https://github.com/opensearch-project/oui/blob/main/src-docs/src/views/markdown_editor/markdown_editor_with_plugins.js#L22 |
@joshuarrrr let me check once. |
@joshuarrrr let me check once. |
@joshuarrrr shall I remove entire block of code under those imports? |
Yeah, remove the import on L22 and on L251, remove that entry from the array. |
@joshuarrrr Let me check once. |
src-docs/src/views/markdown_editor/markdown_editor_with_plugins.js
Outdated
Show resolved
Hide resolved
Signed-off-by: Tarun Samanta <[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.
LGTM
@BSFishy Does this need to be backported? |
This is breaking. No backport needed. (Also of note, I'm pretty sure this is incompatible with Dashboards) |
@BSFishy please follow me on GITHUB. |
This reverts commit 2086980.
This reverts commit 2086980.
This reverts commit 2086980.
This reverts commit 2086980.
This reverts commit 2086980. Signed-off-by: Andrey Myssak <[email protected]> Co-authored-by: Sergey Myssak <[email protected]>
This reverts commit 2086980.
This reverts commit 2086980. Signed-off-by: Andrey Myssak <[email protected]> Co-authored-by: Sergey Myssak <[email protected]>
This reverts commit 2086980.
This reverts commit 2086980.
* Revert "Remove unused chart theme code (#657)" This reverts commit e9a63af. Signed-off-by: Andrey Myssak <[email protected]> Co-authored-by: Sergey Myssak <[email protected]> * Revert "Remove charts theme code (#554)" This reverts commit 2086980. Signed-off-by: Andrey Myssak <[email protected]> Co-authored-by: Sergey Myssak <[email protected]> --------- Signed-off-by: Andrey Myssak <[email protected]> Co-authored-by: Sergey Myssak <[email protected]> Co-authored-by: Josh Romero <[email protected]>
Description
Removed the charts folder with two files (theme.scss and themes.ts).
Issues Resolved
#531
Check List
yarn lint
yarn test-unit
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.