-
Notifications
You must be signed in to change notification settings - Fork 893
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
refactor: simplify theme configuration and defaulting #7625
refactor: simplify theme configuration and defaulting #7625
Conversation
Signed-off-by: Viraj Sanghvi <[email protected]>
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7625 +/- ##
==========================================
+ Coverage 63.74% 63.76% +0.01%
==========================================
Files 3637 3640 +3
Lines 80448 80491 +43
Branches 12792 12802 +10
==========================================
+ Hits 51285 51325 +40
- Misses 26029 26031 +2
- Partials 3134 3135 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Viraj Sanghvi <[email protected]>
Signed-off-by: Viraj Sanghvi <[email protected]>
Signed-off-by: Viraj Sanghvi <[email protected]>
Signed-off-by: Viraj Sanghvi <[email protected]>
❌ Entry Too LongEntry is 165 characters long, which is 65 characters longer than the maximum allowed length of 100 characters. Please revise your entry to be within the maximum length. |
Signed-off-by: Viraj Sanghvi <[email protected]>
Signed-off-by: Viraj Sanghvi <[email protected]>
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/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-7625-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 bcdbbef7b5ef42b10bdf9aa65ef8b96c126dba82
# Push it to GitHub
git push --set-upstream origin backport/backport-7625-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x Then, create a pull request where the |
…ject#7625) --------- Signed-off-by: Viraj Sanghvi <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit bcdbbef)
--------- Signed-off-by: Viraj Sanghvi <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit bcdbbef)
…ject#7625) --------- Signed-off-by: Viraj Sanghvi <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
Reduces the amount of logic needed to add a new theme, reduces the number of files to be touched, + documents the process. There should be no change in experience.
Additionally, it appears we were persisting the label value in some places, while using the version string in others. This worked because we only ever checked that themeTag versions matched 'v7', but v8/next was the else case so it just so happened to work correctly. This tries to use the version string consistently, while keeping the label used for persisting to advanced settings (because I couldn't find an easy way to translate persisted values for existing configurations).
Unfortunately, due to need to run in different contexts, I had to write this to the least common denominator, so changes across the board are still necessary.
Note: There is an existing bug with browser based settings that doesn't respect the user selected setting in the topnav theme selector (even though the selected theme is actually defined). This isn't a released feature and not making things worse, so didn't address. Bug opened: #7689
Issues Resolved
N/A
Screenshot
Testing the changes
Most of the lines were already covered. The user header theme selection doesn't appear to be. Patch codecov was higher so I didn't add, but can if desired.
To test I:
Changelog
CssDistFilename
exports in favor ofthemeCssDistFilenames
in@osd/ui-shared-deps
Check List
yarn test:jest
yarn test:jest_integration