-
Notifications
You must be signed in to change notification settings - Fork 123
Default Text Component Content Edit #3703
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
base: main
Are you sure you want to change the base?
Default Text Component Content Edit #3703
Conversation
WalkthroughAdds a new migrator step (index 6) in shesha-reactjs TextComponent to set a default content value ("Text...") when content is undefined or null, placed after the existing font size adjustment step. No exported/public API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Designer as App Designer
participant Migrator as TextComponent Migrator
participant Step5 as Step 5: Font Size Adjust
participant Step6 as Step 6: Default Content
Designer->>Migrator: Load existing Text props
Migrator->>Step5: Apply font-size migration
Step5-->>Migrator: Updated props
Migrator->>Step6: Apply default content (content ?? "Text...")
Step6-->>Migrator: Updated props
Migrator-->>Designer: Migrated Text props
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
shesha-reactjs/src/designer-components/text/index.tsx (3)
90-91: Localize or centralize the default stringHardcoding "Text..." in English may conflict with localization. Consider sourcing from your i18n layer or a shared constant (e.g., DEFAULT_TEXT_CONTENT) for consistency.
If you confirm the preferred i18n mechanism (e.g., react-intl, i18next, or your internal utility), I can propose a concrete patch.
90-91: Nit: align with repo quote styleUse single quotes to match the surrounding file’s style.
Apply this minimal diff:
- .add<ITextTypographyProps>(6, (prev) => ({ ...prev, content: prev.content ?? "Text..." })) + .add<ITextTypographyProps>(6, (prev) => ({ ...prev, content: prev.content ?? 'Text...' }))
90-91: Verify defaultcontentininitModelfor new Text componentsI wasn’t able to locate a
content: 'Text...'default in theinitModelblock ofshesha-reactjs/src/designer-components/text/index.tsx. To ensure newly added Text components show the placeholder immediately (even if migrators haven’t run), seed the default there before spreading in the user model.• File:
shesha-reactjs/src/designer-components/text/index.tsx
• Add or confirm in theinitModelblock:initModel: (model) => ({ /* other defaults */ content: 'Text...', // default for new instances ...model, }),Please verify and add this if it’s missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shesha-reactjs/src/designer-components/text/index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
📚 Learning: 2025-06-12T16:55:57.638Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
Applied to files:
shesha-reactjs/src/designer-components/text/index.tsx
📚 Learning: 2025-06-10T11:52:51.462Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3374
File: shesha-reactjs/src/generic-pages/settings/dynamic-theme/textsExample.tsx:13-15
Timestamp: 2025-06-10T11:52:51.462Z
Learning: In the Shesha framework's dynamic theme system, the "Global text" Typography element in textsExample.tsx is intentionally left without explicit styling to demonstrate automatic global text color application through the ConfigProvider's colorText token, while the "Default text" element uses explicit theme color styling to show the difference between global and default text color approaches.
Applied to files:
shesha-reactjs/src/designer-components/text/index.tsx
🔇 Additional comments (1)
shesha-reactjs/src/designer-components/text/index.tsx (1)
90-91: Sensible defaulting: preserves intentional empty strings and only backfills null/undefinedUsing nullish coalescing here is correct and safely avoids overwriting empty string content. Placement after the font-size step is fine. This aligns with prior learnings where migrators resetting defaults is intentional; this change is even safer by only filling nullish values.
Summary by CodeRabbit
Bug Fixes
Chores