-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
chore: New Theme Settings Integration #29848
Conversation
WalkthroughThe changes involve a systematic renaming of the term Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChat with CodeRabbit Bot (
|
/build-deploy-preview |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/7326973269. |
if (request.applicationDetail?.themeSettings) { | ||
yield put( | ||
updateApplicationThemeSettingAction( | ||
request.applicationDetail?.themeSetting, | ||
request.applicationDetail?.themeSettings, | ||
), | ||
); | ||
} |
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.
💡 NOTE
Codebase verification is a beta feature.
Occurrences of both themeSetting
and themeSettings
are present in the codebase. It appears that not all instances of themeSetting
were updated to themeSettings
. This inconsistency should be addressed to align with the PR's objectives.
Analysis chain
The renaming from themeSetting
to themeSettings
in the updateApplicationSaga
should be verified across the entire codebase to ensure consistency and that all references to the old term have been updated.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining occurrences of the old term 'themeSetting'.
rg --type tsx 'themeSetting'
# Ensure the new term 'themeSettings' is used consistently.
rg --type tsx 'themeSettings'
Length of output: 114
Script:
#!/bin/bash
# Search for any remaining occurrences of the old term 'themeSetting' without specifying file type.
rg 'themeSetting'
# Ensure the new term 'themeSettings' is used consistently without specifying file type.
rg 'themeSettings'
Length of output: 3428
…theme-settings-integration
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/7327614246. |
Deploy-Preview-URL: https://ce-29848.dp.appsmith.com |
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/7337856893. |
Deploy-Preview-URL: https://ce-29848.dp.appsmith.com |
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/7338018197. |
/ok-to-test |
The provided command lacks any tags. Please execute '/ok-to-test' again, specifying the tags you want to include or use |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/7338080285. |
Deploy-Preview-URL: https://ce-29848.dp.appsmith.com |
state.ui.applications.currentApplication?.applicationDetail?.themeSetting || | ||
defaultThemeSetting | ||
); | ||
return { |
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 do we need these changes?
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.
Just making sure we spread out the theme setting that we get from the backend over the default settings. Previously it was like "If the backend has themeSettings, use that, otherwise use default Settings"
/ok-to-test tags="@tag.Widget, @tag.Settings, @tag.PropertyPane, @tag.Theme" |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/7344841310. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/7344841310. |
Fixes #29262
Summary by CodeRabbit