chore: remove filled icon style setting #36300
Conversation
jsartisan
requested review from
ApekshaBhosale,
sagar-qa007 and
a team
as code owners
github-actions
bot
added
Anvil Pod
WalkthroughThis pull request implements significant changes by removing the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files not reviewed due to no reviewable changes (1)
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? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10844181754. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/Anvil/AppTheming/AnvilAppThemingSnapshot_spec.ts (1)
Line range hint
1-69: Consider making a few minor improvements to the test code.The existing test code follows most of the best practices mentioned in the additional instructions. However, here are a couple of suggestions for further improvement:
Use constants for commonly used values like theme names, density options, sizing options, and corner values. This will make the code more maintainable and less prone to typos.
Consider extracting the repeated snapshot matching logic into a helper function. This will reduce code duplication and make the tests more readable.
For example:
const matchSnapshotsForAllModes = (snapshotName: string, theme?: string) => { anvilSnapshot.matchSnapshotForCanvasMode(snapshotName, theme); anvilSnapshot.matchSnapshotForPreviewMode(snapshotName, theme); anvilSnapshot.matchSnapshotForDeployMode(snapshotName, theme); }; // Usage matchSnapshotsForAllModes("AppTheming"); matchSnapshotsForAllModes("AppTheming", "dark");Keep up the great work with the test code!
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (13)
- app/client/cypress/e2e/Regression/ClientSide/Anvil/AppTheming/AnvilAppThemingSnapshot_spec.ts (1 hunks)
- app/client/cypress/support/Pages/Anvil/AnvilSnapshot.ts (0 hunks)
- app/client/packages/design-system/theming/src/hooks/src/useTheme.tsx (1 hunks)
- app/client/packages/design-system/theming/src/theme/src/types.ts (1 hunks)
- app/client/packages/design-system/theming/src/token/src/TokensAccessor.ts (0 hunks)
- app/client/packages/design-system/theming/src/token/src/types.ts (0 hunks)
- app/client/packages/design-system/widgets/src/components/Icon/src/Icon.tsx (2 hunks)
- app/client/packages/design-system/widgets/src/scripts/build-icons.ts (0 hunks)
- app/client/src/constants/AppConstants.ts (0 hunks)
- app/client/src/pages/AppViewer/index.tsx (1 hunks)
- app/client/src/pages/Editor/Canvas.tsx (0 hunks)
- app/client/src/pages/Editor/WDSThemePropertyPane/constants.ts (0 hunks)
- app/client/src/pages/Editor/WDSThemePropertyPane/index.tsx (0 hunks)
Files not reviewed due to no reviewable changes (8)
- app/client/cypress/support/Pages/Anvil/AnvilSnapshot.ts
- app/client/packages/design-system/theming/src/token/src/TokensAccessor.ts
- app/client/packages/design-system/theming/src/token/src/types.ts
- app/client/packages/design-system/widgets/src/scripts/build-icons.ts
- app/client/src/constants/AppConstants.ts
- app/client/src/pages/Editor/Canvas.tsx
- app/client/src/pages/Editor/WDSThemePropertyPane/constants.ts
- app/client/src/pages/Editor/WDSThemePropertyPane/index.tsx
Files skipped from review due to trivial changes (1)
- app/client/src/pages/AppViewer/index.tsx
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Anvil/AppTheming/AnvilAppThemingSnapshot_spec.ts (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
Additional comments not posted (5)
app/client/packages/design-system/theming/src/theme/src/types.ts (1)
5-5: Great work simplifying the Theme type! 👍The removal of the
iconStyleproperty from theThemetype aligns perfectly with the PR objectives to remove theme icon style settings and eliminate the ability to use filled icons. This change streamlines theThemetype and reflects a shift in how icon styles are managed within the application.It's important to note that this modification may impact how themes are applied or utilized, particularly in relation to icon styling. Developers working with themes should be aware of this change and adjust their code accordingly to handle icon styles separately or through alternative means.
The remaining properties of the
Themetype, such astypographyandcolorMode, are left intact, indicating that the focus of the theme is now more focused without the inclusion of icon styles.Overall, this is a clean and localized change that contributes to the broader goal of simplifying theming and icon management in the codebase. Well done! 🌟
app/client/packages/design-system/widgets/src/components/Icon/src/Icon.tsx (2)
10-10: Great work simplifying the Icon component's props! 👍Removing the
filledproperty aligns perfectly with the PR objectives and streamlines the component's functionality. This change will make the Icon component easier to use and maintain.
26-26: Excellent update to theuseMemodependency array! 🎉Updating the dependency array to only include
nameis the right move after removing thefilledproperty and its related logic. This ensures that the icon is only recomputed when necessary, improving performance.app/client/cypress/e2e/Regression/ClientSide/Anvil/AppTheming/AnvilAppThemingSnapshot_spec.ts (1)
69-69: Great job removing the "6. Icon Style" test case!The removal of this test case aligns perfectly with the PR objective of eliminating the filled icon style settings from the codebase. By removing this test, you are ensuring that the test suite stays up to date with the changes in the application.
app/client/packages/design-system/theming/src/hooks/src/useTheme.tsx (1)
13-13: Great work on simplifying the theming system! 👍The removal of the
iconStyleproperty from theUseThemePropsinterface and the corresponding logic in theuseThemefunction aligns perfectly with the PR objectives and the AI-generated summary. These changes streamline the theming process by reducing the number of properties theuseThemefunction handles, making it more focused and easier to maintain.It's evident that you've carefully considered the implications of this change and have made the necessary adjustments throughout the file. The code remains clean, readable, and follows best practices.
Keep up the excellent work in refining the theming system and improving the overall design and architecture of the application! 🌟
Also applies to: 18-24, 28-102
|
Deploy-Preview-URL: https://ce-36300.dp.appsmith.com |
Fixes appsmithorg#35175 /ok-to-test tags="@tag.Anvil" <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10845022776> > Commit: 9224955 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10845022776&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Anvil` > Spec: > <hr>Fri, 13 Sep 2024 08:02:36 UTC <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Simplified theme management by removing icon style options from the user interface. - **Bug Fixes** - Enhanced clarity in theme handling by refining type usage in various components. - **Chores** - Removed deprecated icon style properties and constants to streamline code and improve maintainability. - Deleted a test case related to icon styles to focus testing efforts elsewhere. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Pawan Kumar <pawankumar@Pawans-MacBook-Pro-2.local>
Fixes #35175
/ok-to-test tags="@tag.Anvil"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10845022776
Commit: 9224955
Cypress dashboard.
Tags:
@tag.AnvilSpec:
Fri, 13 Sep 2024 08:02:36 UTC
Summary by CodeRabbit
New Features
Bug Fixes
Chores