chore: stat widget polish #33696
chore: stat widget polish #33696
Conversation
github-actions
bot
added
Anvil Pod
WalkthroughWalkthroughThe changes involve renaming and enhancing the Changes
Assessment against linked issues
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9207257815. |
|
Deploy-Preview-URL: https://ce-33696.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9219852468. |
|
Deploy-Preview-URL: https://ce-33696.dp.appsmith.com |
| } = props; | ||
|
|
||
| return ( | ||
| <Flex | ||
| alignItems="center" | ||
| className={styles.statbox} | ||
| data-clickable={onClick !== undefined ? "" : undefined} |
There was a problem hiding this comment.
removedclassName={styles.statbox} as styles from src/layoutSystems/anvil/common/styles.module.css were enough.
I added data-clickable just to add styles ( like hover )based on if there is onClick handler available. Let me remove this as we will be doing it separately.
There was a problem hiding this comment.
Removed this prop.
| @@ -1,7 +1,7 @@ | |||
| import type { AnvilConfig } from "WidgetProvider/constants"; | |||
|
|
|||
| export const anvilConfig: AnvilConfig = { | |||
| isLargeWidget: true, | |||
| isLargeWidget: false, | |||
There was a problem hiding this comment.
This is not enough because ResponsiveBehavior should be changed to hug. The idea here is to be able to have multiple StatsWidget in the same zone.
There was a problem hiding this comment.
Fill or Hug? 😵💫 We have the `ResponsiveBehaviour.Fill` for statbox in our `src/widgets/wds/WDSStatsWidget/config/defaultsConfig.ts`
There was a problem hiding this comment.
Sorry, I'm a little confused about the terminology. Fill widget is the correct behavior, but as far as I can see, it does not work correctly.
Here's what we have:
And for example, how inputs works:
So Stats should behave the same way.
| @@ -1,7 +1,7 @@ | |||
| import { WIDGET_TAGS } from "constants/WidgetConstants"; | |||
|
|
|||
| export const metaConfig = { | |||
| name: "Statbox", | |||
| name: "Stats", | |||
There was a problem hiding this comment.
I'm just wondering why the name was changed?
There was a problem hiding this comment.
It was there in list of things we need to do in the polish.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9222033974. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9222073045. |
|
Deploy-Preview-URL: https://ce-33696.dp.appsmith.com |
jsartisan
requested review from
a team and
riodeuno
and removed request for
a team
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9223591801. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (11)
app/client/src/sagas/WidgetAdditionSagas.ts (11)
Line range hint
66-66: Specify a more precise type instead ofany.- tabs: any; + tabs: { [key: string]: TabDetails }; // Assuming TabDetails is an interface representing the structure of each tab.
Line range hint
232-232: Specify a more precise type instead ofany.- propsBlueprint?: WidgetBlueprint; + propsBlueprint?: WidgetBlueprint; // Ensure WidgetBlueprint is properly typed in its definition.
Line range hint
251-251: Use optional chaining to simplify the code.- if (blueprint && blueprint.view) { + if (blueprint?.view) {
Line range hint
273-278: Prefer usingfor...offor iterating over arrays.- childPropsList.forEach((props: GeneratedWidgetPayload) => { + for (const props of childPropsList) {
Line range hint
290-290: Use optional chaining to simplify the code.- if (blueprint && blueprint.operations && blueprint.operations.length > 0) { + if (blueprint?.operations?.length > 0) {
Line range hint
304-304: Avoid using thedeleteoperator; consider setting the property toundefinedor restructuring the object without the property.- delete widget.blueprint; + const { blueprint, ...rest } = widget; + widget = rest;
Line range hint
478-478: Specify a more precise type instead ofany.- const newTabProps: any = getChildTabData(tabProps, { + const newTabProps: TabProps = getChildTabData(tabProps, { // Assuming TabProps is an interface for tab properties.
Line range hint
491-491: Specify a more precise type instead ofany.- const updatedWidgets: CanvasWidgetsReduxState = yield call( + const updatedWidgets: CanvasWidgetsReduxState = yield call( // Ensure CanvasWidgetsReduxState is properly typed in its definition.
Line range hint
501-501: Simplify the computed expression without using a string literal.- const sessionStorageKey = `${widgetType}.${parent.widgetId}.${key}`; + const sessionStorageKey = [widgetType, parent.widgetId, key].join('.');
Line range hint
563-563: Specify a more precise type instead ofany.- let valueFromSession: any = sessionStorage.getItem(sessionStorageKey); + let valueFromSession: string | null = sessionStorage.getItem(sessionStorageKey); // sessionStorage.getItem returns a string or null.
Line range hint
277-277: Avoid reassigning function parameters as it can lead to confusing and hard-to-track bugs.- widget.children = []; + const newChildren = []; + // Use newChildren for further operations and assign it back to widget.children at the end if needed.Also applies to: 292-292
| @@ -552,7 +553,7 @@ export function getWidgetSessionValues( | |||
| } | |||
|
|
|||
| for (const key in configMap) { | |||
| if (configMap[key]) { | |||
| if (configMap[key] != undefined) { | |||
There was a problem hiding this comment.
Use strict inequality operator !== instead of !=.
- if (configMap[key] != undefined) {
+ if (configMap[key] !== undefined) {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if (configMap[key] != undefined) { | |
| if (configMap[key] !== undefined) { |
| @@ -544,6 +544,7 @@ | |||
|
|
|||
| let widgetType = type; | |||
| const configMap = WidgetFactory.widgetConfigMap.get(type); | |||
|
|
|||
| const widgetSessionValues: any = {}; | |||
There was a problem hiding this comment.
Specify a more precise type instead of any.
- const widgetSessionValues: any = {};
+ const widgetSessionValues: Record<string, unknown> = {}; // Use a more specific type if possible.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const widgetSessionValues: any = {}; | |
| const widgetSessionValues: Record<string, unknown> = {}; // Use a more specific type if possible. |
|
Deploy-Preview-URL: https://ce-33696.dp.appsmith.com |
jsartisan
force-pushed
the
chore/statbox-polish
branch
from
807d857 to
46a4270
Compare
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9250475616. |
| constructor(props: StatsWidgetProps) { | ||
| super(props); | ||
| } |
There was a problem hiding this comment.
Remove unnecessary constructor.
- constructor(props: StatsWidgetProps) {
- super(props);
- }The constructor in this class is redundant since it only calls super(props) and does nothing else. Removing it will clean up the code without affecting functionality.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| constructor(props: StatsWidgetProps) { | |
| super(props); | |
| } |
|
Deploy-Preview-URL: https://ce-33696.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9251320197. |
|
Deploy-Preview-URL: https://ce-33696.dp.appsmith.com |
Fixes #33397
/ok-to-test tags="@tag.Anvil"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9257919530
Commit: 30c8b31
Cypress dashboard url: Click here!