[CCI] Revert changes to chart themes #831
Conversation
andreymyssak
requested review from
seanneumann,
AMoo-Miki,
rednaksi91,
KrooshalUX,
BSFishy,
bandinib-amzn,
ashwin-pc,
joshuarrrr and
SergeyMyssak
as code owners
andreymyssak
force-pushed
the
810-revert-chart-themes-changes
branch
from
fcf74bc to
1c44f3d
Compare
|
@AMoo-Miki do we still want this? If it's not merged, OSD builds will be broken on main. I suppose I'm not super opposed to merging it, but we still don't officially support Elastic Charts, so having this code in the repo feels kinda wrong |
|
Logically, these changes should be reverted in 1.x branches only since they are breaking changes in the 1.x releases. By that logic, we shouldn't revert these in |
These were never backported to |
I want to hear @joshuarrrr on this as it will impact his work the most. |
In OpenSearch Dashboards today, we actually do still have a dependency on Elastic Charts - it's the charting library used by TSVB visualizations and a couple OpenSearch project dashboard plugins: https://github.com/search?q=repo%3Aopensearch-project%2FOpenSearch-Dashboards%20%40elastic%2Fcharts&type=code As for these chart themes, they get loaded as part of OpenSearch Dashboards So it's not really safe to remove these until the "Migrate Elastic Chart-rendered components to Vega-Lite" section of opensearch-project/OpenSearch-Dashboards#2819 is completed |
There was a problem hiding this comment.
This all looks good - note that some of the theme files were renamed (such as https://github.com/AMoo-Miki/oui/blob/c7f7c70c0f106e88c8e322962bb21bb601104320/src/themes/oui-next/oui_next_colors_dark.scss) in #835 - we'll want to make sure each theme file currently existing in main has the
import '../../src/themes/charts/theme';
line after the elastic charts theme import
| @@ -246,7 +248,7 @@ const ChartMarkdownRenderer = ({ palette, categories }) => { | |||
| return ( | |||
| <Chart size={{ height: 320 }}> | |||
| <Settings | |||
| theme={[customColors]} | |||
| theme={[customColors, OUI_CHARTS_THEME_LIGHT]} | |||
There was a problem hiding this comment.
Note to self - open issue to look into migrating/removing elastic chart usage from OUI itself.
This reverts commit e9a63af. Signed-off-by: Andrey Myssak <[email protected]> Co-authored-by: Sergey Myssak <[email protected]>
This reverts commit 2086980. Signed-off-by: Andrey Myssak <[email protected]> Co-authored-by: Sergey Myssak <[email protected]>
andreymyssak
force-pushed
the
810-revert-chart-themes-changes
branch
from
1c44f3d to
73fefe1
Compare
|
Once #855 is merged, this PR will be ready for review |
|
@BSFishy, can you advise me in which section of CHANGELOG.md I should put this change? |
I think if the original ones had changelog entries, we'd have put them in |
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/oui/backport-1.x 1.x
# Navigate to the new working tree
pushd ../.worktrees/oui/backport-1.x
# Create a new branch
git switch --create backport/backport-831-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f8d56ba11811d600eb74de9d6b71e8ab9d16a1d8
# Push it to GitHub
git push --set-upstream origin backport/backport-831-to-1.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/oui/backport-1.xThen, create a pull request where the |
|
No need for backports, as, to the best of my understanding, the original changes were never backported. |
Description
2086980 and e9a63af have been reverted to not break integrations with OpenSearch Dashboards.
Issues Resolved
#810
Check List
yarn lintyarn test-unitBy 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.