chore: Add documentation for property pane config #36880
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
|
jsartisan
changed the title
github-actions
bot
added
the
skip-changelog
![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 (9)
app/client/src/constants/PropertyControlConstants.tsx (7)
59-63: Well done on adding theidproperty, class!The addition of the
idproperty with its detailed JSDoc comment is a commendable effort. It's like adding a name tag to each student in our class – it helps us keep track of everyone!However, let's make it even better. Can you tell me what type of string this
idis? Is it a UUID, a sequential number, or something else? Adding this information would be like specifying what kind of name tag we're using.Consider adding more specificity to the JSDoc comment:
/** * Unique identifier for the control. Used for internal tracking and debugging. * It is added by `addPropertyConfigIds` function in `WidgetProvider/factory/helpers.ts`. * @type {string} The id is typically a [specify the format, e.g., UUID, sequential number]. */ id?: string;
85-89: Excellent addition of thecustomJSControlproperty, students!This new property is like giving our class a special project option. It allows for more advanced and customized controls, which is fantastic for those students who want to go the extra mile!
The example you've provided with the
COMPUTE_VALUEcontrol is very helpful. It's like explaining that this special project could be used in a science fair to showcase complex experiments.To make it even clearer for all our students, let's add a bit more context:
/** * Used in special cases where we want a custom control instead of a regular text input control. * This allows for more complex interactions or computations within the property pane. * @example * The `COMPUTE_VALUE` control is used in the table widget to provide access to the currentRow * in controls within the table columns, enabling row-specific computations. */ customJSControl?: string;This way, even our less experienced students will understand the power and purpose of this property!
90-94: A gold star for adding thecontrolTypeproperty!This property is like choosing which subject we're teaching in class. It's essential for determining how we interact with our students (or in this case, how users interact with the property).
Your comment pointing to where we can find the full list of supported controls is very helpful. It's like having a syllabus that tells us all the subjects we can teach!
Let's make it even more informative for our fellow teachers:
/** * Specifies the type of property control to be used. * This determines the UI component and behavior of the property in the property pane. * @type {ControlType} * @required * @see {ControlTypes} for the full list of supported controls in `app/client/src/components/propertyControls/index.ts`. */ controlType: ControlType;This way, we're not just telling where to find the information, but also emphasizing its importance and how it relates to the UI. It's like giving a brief introduction to the subject before diving into the lesson!
153-156: Good job adding thevalidationproperty, but let's expand our lesson plan!Adding validation is like setting the grading criteria for our assignments. It's crucial for ensuring that the data our students (users) input meets our standards.
However, your current explanation is a bit too brief. It's like telling students "This is a test" without explaining what it's testing or how it will be graded.
Let's provide a more comprehensive explanation:
/** * Specifies the validation rules for the property. * This helps ensure data integrity and provides immediate feedback to users. * @type {ValidationConfig} * @optional * @example * validation: { * type: ValidationTypes.NUMBER, * params: { * min: 0, * max: 100, * default: 50 * } * } * @see {ValidationConfig} for the full structure of validation options. */ validation?: ValidationConfig;This expanded comment is like giving a detailed rubric. It explains what the validation does, provides an example, and points to where more information can be found. This will help our fellow teachers (developers) understand and use the validation properly!
157-166: Excellent addition of theadditionalAutoCompleteproperty!This property is like giving our students a customized dictionary for their assignments. It helps them complete their work more efficiently by suggesting relevant terms.
Your explanation is good, but let's make it even more helpful for our fellow teachers.
Let's enhance the JSDoc comment with an example:
/** * Callback function to provide additional autocomplete data for the autocomplete list. * This is useful for adding context-specific suggestions that aren't part of the standard autocomplete. * * @param props - Current widget properties * @returns {AdditionalDynamicDataTree} Additional autocomplete data * * @example * additionalAutoComplete: (props) => ({ * customCategory: { * data1: {}, * data2: {}, * }, * }) */ additionalAutoComplete?: (props: any) => AdditionalDynamicDataTree;This expanded comment is like providing a sample lesson plan along with the teaching guidelines. It shows other teachers (developers) exactly how they might use this feature in their own "classrooms" (widgets).
172-176: A round of applause for adding thedynamicDependenciesproperty!This property is like having a flexible curriculum that adapts to each student's needs. It allows us to determine what prerequisites are needed for a lesson in real-time.
Your explanation does a good job of distinguishing this from the static
dependencies. However, let's make it even clearer for our fellow educators.Let's enhance the JSDoc comment:
/** * Determines dependencies dynamically during the rendering of the property control. * Unlike static `dependencies`, this allows for context-sensitive dependency resolution. * * @param {WidgetProps} widget - The current widget properties * @returns {string[]} An array of property names that this property depends on * * @example * dynamicDependencies: (widget) => { * return widget.isAdvancedMode ? ['advancedProperty1', 'advancedProperty2'] : ['basicProperty']; * } */ dynamicDependencies?: (widget: WidgetProps) => string[];This expanded comment is like providing a detailed lesson plan that explains not just what we're doing, but why and how. The example shows other teachers (developers) how they might use this feature to create more adaptive and context-aware widgets.
217-220: Good effort on adding thedefaultValueproperty, but let's flesh out our lesson plan!Adding a default value is like providing a starting point for our students' assignments. It's very helpful, especially for complex properties.
However, your current explanation is a bit too concise. It's like telling students "Start here" without explaining why or how this starting point was chosen.
Let's provide a more comprehensive explanation:
/** * Specifies the default value for the property. * This value is used when the property is first initialized or reset. * * @type {unknown} * @optional * * @example * // For a number property * defaultValue: 0 * * // For a string property * defaultValue: "Hello, World!" * * // For a boolean property * defaultValue: false * * @note The type of the default value should match the expected type of the property. */ defaultValue?: unknown;This expanded comment is like providing a detailed introduction to our lesson. It explains what the default value does, provides examples for different types of properties, and includes a note about type matching. This will help our fellow teachers (developers) understand how to use default values effectively in their own "lessons" (widgets)!
app/client/src/utils/PropertyControlFactory.tsx (2)
16-16: Correct the grammatical error in the class documentation.Remember, clear and grammatically correct documentation is essential for maintainability. In line 16, it should read 'This class manages' instead of 'This classes manages' to ensure proper grammar.
Line range hint
71-74: Refactor exception handling to use a custom Error subclass.Proper exception handling is crucial for robustness. Currently, you're throwing an object that implements an interface, which won't preserve the error's stack trace and runtime properties. It's better to extend the built-in
Errorclass to create a custom exception. This way, you maintain all the benefits of standard error handling.Apply this diff to refactor the exception handling:
+export class ControlCreationException extends Error { + constructor(message: string) { + super(message); + this.name = "ControlCreationException"; + } +} ... - const ex: ControlCreationException = { - message: - "Control Builder not registered for control type " + - controlData.controlType, - }; - throw ex; + throw new ControlCreationException( + "Control Builder not registered for control type " + + controlData.controlType, + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/constants/PropertyControlConstants.tsx (4 hunks)
- app/client/src/utils/PropertyControlFactory.tsx (1 hunks)
🧰 Additional context used
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/client/src/constants/PropertyControlConstants.tsx (4)
20-86: Excellent work on documenting the PropertyPaneSectionConfig interface! 📚
Class, I'm pleased to see such thorough documentation! However, let's enhance it further with some practical examples to help our fellow developers better understand the usage.
Consider adding example usage for the sectionName and children properties. Here's a suggested addition to the JSDoc:
/**
* Title displayed at the top of a collapsible section in the property pane.
+ *
+ * @example
+ * sectionName: "Basic Configuration"
*/
sectionName: string;
/**
* Array of properties that should go inside the section.
+ *
+ * @example
+ * children: [
+ * {
+ * controlType: "INPUT_TEXT",
+ * propertyName: "title",
+ * label: "Title",
+ * }
+ * ]
*/
children: PropertyPaneConfig[];Line range hint 191-197: Time to address those TODO comments, class! 🎯
I notice several eslint-disable comments with TODOs. Let's improve type safety by replacing any with proper types.
Consider replacing the any type with a more specific type:
updateHook?: (
- // TODO: Fix this the next time the file is edited
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- props: any,
+ props: WidgetProps,
propertyName: string,
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- propertyValue: any,
+ propertyValue: unknown,
) => Array<PropertyUpdates> | undefined;
hidden?: (
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- props: any,
+ props: WidgetProps,
propertyPath: string
) => boolean;Also applies to: 205-207
Line range hint 366-376: Let's improve our type definitions here, students! 📝
The validation function parameters could be more specific than any.
Let's enhance type safety:
fn?: (
value: unknown,
- // TODO: Fix this the next time the file is edited
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- props: any,
+ props: WidgetProps,
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- _?: any,
+ _?: unknown,
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- moment?: any,
+ moment?: moment.Moment,
) => ValidationResponse;Don't forget to import the moment type if you make this change:
import type { Moment } from 'moment';Line range hint 1-421: Outstanding documentation effort! Let's add some cross-references! 🌟
The documentation is thorough, but we can make it even better by adding cross-references between related interfaces.
Consider adding @see tags to link related properties and interfaces. For example:
/**
* Validation configuration for the property
+ * @see ValidationConfig
+ * @see ValidationConfigParams
*/
validation?: ValidationConfig;This will help developers navigate the documentation more effectively.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/constants/PropertyControlConstants.tsx (5 hunks)
- app/client/src/pages/Editor/PropertyPane/PropertyControlsGenerator.tsx (1 hunks)
🔇 Additional comments (1)
app/client/src/pages/Editor/PropertyPane/PropertyControlsGenerator.tsx (1)
133-133: Class, let's examine this type assertion removal!
I notice you've removed the explicit type casting from searchResults. While this suggests that TypeScript can now correctly infer the types, let's verify that the type safety is maintained.
Let's run a quick type-check to ensure everything is in order:
Remember class, type safety is crucial! If the types match correctly, this change actually improves code clarity by removing redundant type assertions.
|
This PR has been closed because of inactivity. |
|
@riodeuno Can you check if it's okay to merge this? We can improve the comments further later on. |
|
@jsartisan yes, please go ahead with this. I'm approving it |
/ok-to-test tags="@tag.Anvil" <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new `PropertyControlFactory` class for managing property controls. - Added new properties to the `PropertyPaneControlConfig` interface, enhancing customization and validation options. - Implemented methods for creating and registering control builders, improving the flexibility of property controls. - **Documentation** - Enhanced JSDoc comments for existing properties and methods, providing clearer guidance on their usage. - Updated documentation for the `PropertyPaneSectionConfig`, `PropertyPaneControlConfig`, and `ValidationConfigParams` interfaces to clarify their functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11716829822> > Commit: 62df826 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11716829822&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Anvil` > Spec: > <hr>Thu, 07 Nov 2024 05:15:44 UTC <!-- end of auto-generated comment: Cypress test results -->
/ok-to-test tags="@tag.Anvil"
Summary by CodeRabbit
New Features
PropertyControlFactoryclass for managing property controls.PropertyPaneControlConfiginterface, enhancing customization and validation options.Documentation
PropertyPaneSectionConfig,PropertyPaneControlConfig, andValidationConfigParamsinterfaces to clarify their functionality.Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11716829822
Commit: 62df826
Cypress dashboard.
Tags:
@tag.AnvilSpec:
Thu, 07 Nov 2024 05:15:44 UTC