chore: Refactor WDSTableWidget with basic features #33286
Conversation
jsartisan
requested review from
a team and
riodeuno
and removed request for
a team
github-actions
bot
added
the
skip-changelog
WalkthroughWalkthroughThe updates focus on refining the design system and widget components across the client app, enhancing usability and consistency. Key changes include the correction of theme property assignments, the expansion of component exportations, and the simplification of component properties and configurations. Additionally, adjustments in table widget components and sagas improve functionality and code clarity, aligning with modern best practices. Changes
This table summarizes the broad scope of changes, focusing on enhancing component functionality, simplifying configurations, and improving code maintainability across various parts of the design system and widgets. Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review due to trivial 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Out of diff range and nitpick comments (9)
app/client/src/widgets/wds/WDSTableWidget/component/cellComponents/HeaderCell.tsx (3)
Line range hint
1-1: Ensure consistent import ordering and grouping.Consider grouping and ordering imports to improve readability and maintainability. Typically, external libraries should be imported first, followed by internal modules.
Line range hint
78-79: Consider using CSS for styling instead of inline styles.Inline styles can lead to code duplication and harder maintenance. It's generally a good practice to use CSS classes for styling, especially for static styles.
Line range hint
114-114: Ensure consistent padding in the header.The padding-inline-end is not consistently set, which might lead to visual inconsistency. Consider setting a uniform padding for both start and end.
app/client/src/widgets/wds/WDSTableWidget/component/styles.module.css (3)
78-79: Check for consistent padding settings in the header.The padding settings for
padding-inline-startandpadding-inline-endare different. This might lead to visual inconsistency. Consider setting uniform padding for both.
145-145: Check for consistent height settings across similar elements.The
min-block-sizeis set explicitly here, which might lead to inconsistencies with other similar elements. Consider reviewing the height settings across the project for uniformity.
218-218: Check for consistent alignment settings across similar elements.The alignment settings for
align-itemsandjustify-contentare set tocenterusing!important. This might override other styles and lead to maintenance issues. Consider using a more flexible approach if possible.app/client/src/widgets/wds/WDSTableWidget/component/Table.tsx (1)
Line range hint
280-388: Refactor the rendering logic to improve maintainability.The
Tablecomponent's rendering logic is quite complex, especially with the conditional rendering ofStaticTableandVirtualTable. Consider breaking down this component into smaller, more manageable sub-components. This could improve readability and maintainability. For example, the decision logic for renderingStaticTablevs.VirtualTablecould be encapsulated in a separate component or hook.app/client/src/widgets/wds/WDSTableWidget/widget/index.tsx (2)
2-2: Consider removing unused imports to clean up the code.
Line range hint
321-375: This methodcreateTablePrimaryColumnsis quite complex and could benefit from refactoring to improve readability and maintainability. Consider breaking it down into smaller, more focused functions.
| @@ -174,7 +174,7 @@ export const discardButtonStyleConfig = { | |||
| { | |||
| propertyName: "discardBorderRadius", | |||
| label: "Border radius", | |||
| customJSControl: "TABLE_COMPUTE_VALUE", | |||
|
|
|||
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has revealed that the project uses a CSS variable --border-radius-elevation-3 in multiple widgets, indicating a design system in place. However, there are also several instances where border-radius values are hardcoded, including 4px and 36px. This suggests that while there is a design system, there are exceptions or inconsistencies in its application across different parts of the project.
Given this information, it appears that the border-radius settings are not entirely consistent throughout the project. This could be intentional (for specific design reasons) or an oversight.
Action Points:
- Review the use of hardcoded border-radius values to ensure they are intentional and documented.
- Consider standardizing the use of the CSS variable across all components for consistency.
Analysis chain
Verify the border radius options for consistency.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the border radius options are consistent with the project's design system.
# Test: Search for border radius settings in the project's stylesheets. Expect: Consistent border radius settings.
rg --type css 'border-radius' app/client/src/widgets/wds/WDSTableWidget/component/styles.module.css
Length of output: 174
Script:
#!/bin/bash
# Search for the definition of the CSS variable `--border-radius-elevation-3`
rg --type css 'var\(--border-radius-elevation-3\)' -A 1 app/client/src/
# Search for other occurrences of hardcoded border-radius values across the project
rg --type css 'border-radius: [0-9]+px;' app/client/src/
Length of output: 931
| @@ -1,3 +1,6 @@ | |||
| /* TODO(pawan): can we access css module variable from packages/widgets */ | |||
There was a problem hiding this comment.
Clarify the TODO comment or resolve it before merging.
The TODO comment about accessing CSS module variables seems to be a placeholder. Would you like me to help resolve this or should it be tracked in a separate issue?
| @@ -107,7 +111,7 @@ | |||
| inset: 0; | |||
| display: flex; | |||
| align-items: items; | |||
| padding-inline: var(--outer-spacing-2); | |||
| padding-inline: var(--outer-spacing-3); | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has revealed a wide variety of padding settings across different CSS files in the project. The padding values and the variables used for padding are not consistent, as they vary significantly from one file to another. This includes different padding styles (e.g., padding-inline, padding-block, padding-left, padding-right) and different variable names and values being used across various components and layouts.
Given the evidence from the script outputs, it is clear that the padding settings are not consistent across the project. This could potentially lead to inconsistencies in the UI's look and feel across different parts of the application.
Action Required:
-
Review the project's design system or CSS architecture to establish a consistent approach for padding across the project.
-
Consider defining a set of standard padding variables and apply these consistently across all CSS files.
Analysis chain
Ensure that the padding settings are consistent across the project.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if padding settings are consistent in the project's CSS files.
# Test: Search for padding settings in the project's stylesheets. Expect: Consistent padding settings.
rg --type css 'padding' app/client/src/widgets/wds/WDSTableWidget/component/styles.module.css
Length of output: 389
Script:
#!/bin/bash
# Description: Search for all padding settings across all CSS files in the project to verify consistency.
# Search for padding settings in all CSS files and display unique occurrences to check for consistency.
rg --type css 'padding' -g '*.css' | sort | uniq -c
Length of output: 94262
| [data-column-type="url"], | ||
| [data-column-type="date"] | ||
| ) { | ||
| padding: var(--outer-spacing-4) var(--outer-spacing-3); |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has revealed that there are multiple unique padding settings used across the project's CSS files. This indicates that the padding settings are not consistent throughout the project. Here are the findings:
padding: var(--inner-spacing-2)padding: var(--outer-spacing-1)padding: var(--outer-spacing-4)
Given these results, it is clear that the padding settings vary, which may not align with the goal of maintaining consistency as mentioned in the review comment.
Action Required:
- Review and possibly standardize the padding variables used across different CSS files to ensure a consistent look and feel throughout the application.
Analysis chain
Ensure that the padding settings are consistent across the project.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if padding settings are consistent in the project's CSS files.
# Test: Search for padding settings in the project's stylesheets. Expect: Consistent padding settings.
rg --type css 'padding' app/client/src/widgets/wds/WDSTableWidget/component/styles.module.css
Length of output: 389
Script:
#!/bin/bash
# Description: Check for padding settings across all CSS files in the project to verify consistency.
# Search for padding settings in all CSS files and display unique occurrences to check for consistency.
rg --type css 'padding: var\(--[^)]+\)' -o --no-filename | sort | uniq -c
Length of output: 195
| accentColor?: string; | ||
| borderRadius?: string; | ||
| }>` | ||
| export const CellCheckboxWrapper = styled(CellWrapper)` |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has revealed that the z-index settings within the TableStyledWrappers.tsx file and other related CSS files in the widgets directory show a variety of values. These include fixed values like z-index: 1; and z-index: 100;, as well as dynamic values such as z-index: ${Layers.modalWidget}; and conditional values like z-index: ${(props) => (props.sticky ? Layers.dragPreview : 0)};.
Given this variety, it appears that the z-index settings are not entirely consistent across the project, possibly due to different requirements or contexts within the widget components. Each usage of z-index might be justified based on specific UI layering needs.
Recommendation:
-
Review the specific requirements for
z-indexin each component to ensure they meet the intended design and functionality. -
Consider standardizing
z-indexvalues where possible to maintain consistency and avoid layering conflicts. -
Locations with varied
z-indexusage:app/client/src/widgets/wds/WDSTableWidget/component/TableStyledWrappers.tsxapp/client/src/widgets/wds/WDSTableWidget/component/styles.module.css
Analysis chain
Ensure that the z-index setting is intentional and consistent.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the z-index setting for `CellCheckboxWrapper` is consistent across the project.
# Test: Search for z-index settings in the project's stylesheets. Expect: Consistent usage of z-index.
rg --type css 'z-index' app/client/src/widgets/wds/WDSTableWidget/component/TableStyledWrappers.tsx
Length of output: 213
Script:
#!/bin/bash
# Description: Search for z-index usage in similar styled components across the project to understand context and consistency.
# Search for z-index in all CSS and styled component files in the widgets directory, excluding the current file.
rg --type css 'z-index' app/client/src/widgets/ --glob '!**/TableStyledWrappers.tsx'
Length of output: 344
|
/build-deploy-preview skip-tests=true env=release |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9012740346. |
|
Deploy-Preview-URL: https://appsmith-r85ruaeba-get-appsmith.vercel.app 🚨 Note: The release environment runs EE code, so using a frontend-only DP on this repo, will |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9013842499. |
|
Deploy-Preview-URL: https://ce-33286.dp.appsmith.com |
| @@ -201,22 +201,24 @@ class PrimaryColumnsControlV2 extends BaseControl<ControlProps, State> { | |||
| const isFocused = | |||
| !_.isNull(this.state.focusedIndex) && | |||
| _.includes(this.state.duplicateColumnIds, column?.id); | |||
|
|
|||
There was a problem hiding this comment.
The pattern for this should be that TableWidget module exports the control, which is then registered by Appsmith. This way, this control can be as specific as necessary to the TableWidget.
@jsartisan What do you think?
There was a problem hiding this comment.
Yeah, I agree. We have to make a system where widgets can also register custom controls in the control registry.
|
/build-deploy-preview skip-tests=true |
| /> | ||
| </EdtiableCheckboxWrapper> | ||
| )} | ||
| {this.props.widgetProperties.type !== "WDS_TABLE_WIDGET" && |
There was a problem hiding this comment.
Hiding the editable checkbox for WDSTableWidget
There was a problem hiding this comment.
we don't want to show the editable icon for wds table widget. This control is being used in TableWidgetV2 as well. So had to add a check. Another way is to create the whole control and use it in table widget.
There was a problem hiding this comment.
Another way is to create the whole control and use it table widget.
This is the preferred approach, and we have to promote a complete separation from the legacy.
| > | ||
| Add new column | ||
| </Button> | ||
| {this.props.widgetProperties.type !== "WDS_TABLE_WIDGET" && ( |
There was a problem hiding this comment.
Hiding the Add the new column button for WDSTableWidget
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9015832676. |
| getQueryGenerationConfig: () => { | ||
| return { | ||
| select: { | ||
| limit: `${widget.widgetName}.pageSize`, | ||
| where: `${widget.widgetName}.searchText`, | ||
| offset: `${widget.widgetName}.pageOffset`, | ||
| orderBy: `${widget.widgetName}.sortOrder.column`, | ||
| sortOrder: `${widget.widgetName}.sortOrder.order !== "desc"`, | ||
| limit: `100`, |
There was a problem hiding this comment.
Consider making the query limit configurable.
It's generally a good practice to allow configuration of such parameters to accommodate different use cases. You might want to add a parameter to this method to specify the limit or fetch it from a configuration file or environment variable.
| serverSidePaginationEnabled: false, | ||
| enableClientSideSearch: true, |
There was a problem hiding this comment.
Consider making pagination and search settings configurable.
Hardcoding serverSidePaginationEnabled and enableClientSideSearch may not be suitable for all scenarios. It would be beneficial to allow these settings to be configurable to adapt to different use cases. Additionally, consider adding documentation or comments explaining the choice of setting isVisibleDownload to false.
|
Deploy-Preview-URL: https://ce-33286.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9016330926. |
|
Deploy-Preview-URL: https://ce-33286.dp.appsmith.com |
| return value; | ||
| } | ||
| }, [value, decimals, currencyCode, notation, thousandSeparator, columnType]); | ||
|
|
||
| if (isCellEditMode) { | ||
| const [inputType, inputHTMLType]: [InputTypes, "TEXT" | "NUMBER"] = (() => { | ||
| switch (columnType) { | ||
| case ColumnTypes.NUMBER: | ||
| return [InputTypes.NUMBER, "NUMBER"]; | ||
| case ColumnTypes.CURRENCY: | ||
| return [InputTypes.CURRENCY, "NUMBER"]; | ||
| default: | ||
| return [InputTypes.TEXT, "TEXT"]; | ||
| } | ||
| })(); | ||
|
|
||
| const additionalProps: Record<string, unknown> = {}; | ||
|
|
||
| if (inputType === InputTypes.CURRENCY) { | ||
| additionalProps.inputHTMLType = "NUMBER"; | ||
| additionalProps.leftIcon = ( | ||
| <CurrencyTypeDropdown | ||
| accentColor={accentColor} | ||
| allowCurrencyChange={false} | ||
| options={CurrencyDropdownOptions} | ||
| selected={currencyCode} | ||
| widgetId={widgetId} | ||
| /> | ||
| ); | ||
| additionalProps.shouldUseLocale = true; | ||
| additionalProps.decimals = decimals; | ||
| } | ||
|
|
||
| editor = ( | ||
| <InlineCellEditor | ||
| accentColor={accentColor} | ||
| additionalProps={additionalProps} | ||
| allowCellWrapping={allowCellWrapping} | ||
| autoFocus={!isNewRow} | ||
| compactMode={compactMode} | ||
| inputHTMLType={inputHTMLType} | ||
| inputType={inputType} | ||
| isEditableCellValid={isEditableCellValid} | ||
| multiline={isMultiline} | ||
| onChange={editEvents.onChange} | ||
| onDiscard={editEvents.onDiscard} | ||
| onSave={editEvents.onSave} | ||
| paddedInput={isNewRow} | ||
| textSize={textSize} | ||
| validationErrorMessage={validationErrorMessage} | ||
| value={value} | ||
| verticalAlignment={verticalAlignment} | ||
| widgetId={widgetId} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <Container | ||
| cellBackground={cellBackground} | ||
| className="t--table-text-cell" | ||
| isCellEditMode={isCellEditMode} | ||
| verticalAlignment={verticalAlignment} | ||
| <Text | ||
| color={cellColor === "default" ? undefined : cellColor} | ||
| isBold={isBold} | ||
| isItalic={isItalic} | ||
| lineClamp={lineClamp} | ||
| title={value} | ||
| variant="body" | ||
| > | ||
| <BasicCell | ||
| accentColor={accentColor} | ||
| allowCellWrapping={allowCellWrapping} | ||
| cellBackground={cellBackground} | ||
| columnType={columnType} | ||
| compactMode={compactMode} | ||
| disabledEditIcon={disabledEditIcon} | ||
| disabledEditIconMessage={disabledEditIconMessage} | ||
| fontStyle={fontStyle} | ||
| hasUnsavedChanges={hasUnsavedChanges} | ||
| horizontalAlignment={horizontalAlignment} | ||
| isCellDisabled={isCellDisabled} | ||
| isCellEditMode={isCellEditMode} | ||
| isCellEditable={isCellEditable} | ||
| isCellVisible={isCellVisible} | ||
| isHidden={isHidden} | ||
| onEdit={editEvents.onEdit} | ||
| ref={contentRef} | ||
| tableWidth={tableWidth} | ||
| textColor={textColor} | ||
| textSize={textSize} | ||
| url={columnType === ColumnTypes.URL ? props.value : null} | ||
| value={formattedValue} | ||
| verticalAlignment={verticalAlignment} | ||
| /> | ||
| {editor} | ||
| </Container> | ||
| {value} | ||
| </Text> |
There was a problem hiding this comment.
Review the implementation of PlainTextCell for potential improvements.
The function PlainTextCell handles several conditional styles and properties. While the current implementation is functional, consider using a helper function to manage style-related logic, which could improve readability and maintainability.
+ function resolveTextProps({ cellColor, isBold, isItalic, allowCellWrapping }) {
+ return {
+ color: cellColor === "default" ? undefined : cellColor,
+ isBold,
+ isItalic,
+ lineClamp: allowCellWrapping ? undefined : 1,
+ };
+ }
+
function PlainTextCell(props: PlainTextCellProps & BaseCellComponentProps) {
const { value } = props;
const textProps = resolveTextProps(props);
return (
<Text
{...textProps}
title={value}
variant="body"
>
{value}
</Text>
);
}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.
| function PlainTextCell(props: PlainTextCellProps & BaseCellComponentProps) { | |
| const { allowCellWrapping, cellColor, isBold, isItalic, value } = props; | |
| const lineClamp = allowCellWrapping ? undefined : 1; | |
| if (value && columnType === ColumnTypes.URL && displayText) { | |
| text = displayText; | |
| } else if (columnType === ColumnTypes.CURRENCY) { | |
| text = value ?? ""; | |
| } else if (!isNil(value) && (!isNumber(value) || !isNaN(value))) { | |
| text = (value as string).toString(); | |
| } else { | |
| text = ""; | |
| } | |
| return text; | |
| } | |
| function getContentHeight(ref: RefObject<HTMLDivElement>) { | |
| return ( | |
| !!ref.current?.offsetHeight && | |
| ref.current?.offsetHeight / CELL_WRAPPER_LINE_HEIGHT > 1 | |
| ); | |
| } | |
| function PlainTextCell( | |
| props: RenderDefaultPropsType & editPropertyType & RenderCurrencyPropsType, | |
| ) { | |
| const { | |
| accentColor, | |
| alias, | |
| allowCellWrapping, | |
| cellBackground, | |
| columnType, | |
| compactMode, | |
| currencyCode, | |
| decimals, | |
| disabledEditIcon, | |
| disabledEditIconMessage, | |
| displayText, | |
| fontStyle, | |
| hasUnsavedChanges, | |
| horizontalAlignment, | |
| isCellDisabled, | |
| isCellEditable, | |
| isCellEditMode, | |
| isCellVisible, | |
| isEditableCellValid, | |
| isHidden, | |
| isNewRow, | |
| notation, | |
| onCellTextChange, | |
| onSubmitString, | |
| rowIndex, | |
| tableWidth, | |
| textColor, | |
| textSize, | |
| thousandSeparator, | |
| toggleCellEditMode, | |
| validationErrorMessage, | |
| verticalAlignment, | |
| widgetId, | |
| } = props; | |
| let value = props.value; | |
| const editEvents = useMemo( | |
| () => ({ | |
| onChange: (value: EditableCell["value"], inputValue: string) => | |
| onCellTextChange(value, inputValue, alias), | |
| onDiscard: () => | |
| toggleCellEditMode( | |
| false, | |
| rowIndex, | |
| alias, | |
| value, | |
| "", | |
| EditableCellActions.DISCARD, | |
| ), | |
| onEdit: () => toggleCellEditMode(true, rowIndex, alias, value), | |
| onSave: () => | |
| toggleCellEditMode( | |
| false, | |
| rowIndex, | |
| alias, | |
| value, | |
| onSubmitString, | |
| EditableCellActions.SAVE, | |
| ), | |
| }), | |
| [ | |
| onCellTextChange, | |
| toggleCellEditMode, | |
| value, | |
| rowIndex, | |
| alias, | |
| onSubmitString, | |
| ], | |
| ); | |
| value = getCellText(value, columnType, displayText); | |
| const contentRef = useRef<HTMLDivElement>(null); | |
| let editor; | |
| const [isMultiline, setIsMultiline] = useState(false); | |
| useEffect(() => { | |
| if (isCellEditMode) { | |
| fastdom.measure(() => { | |
| setIsMultiline(getContentHeight(contentRef)); | |
| }); | |
| } | |
| }, [value, isCellEditMode]); | |
| const currency = useMemo( | |
| () => CurrencyDropdownOptions.find((d) => d.value === currencyCode), | |
| [currencyCode], | |
| ); | |
| const formattedValue = useMemo(() => { | |
| if (columnType === ColumnTypes.CURRENCY) { | |
| try { | |
| const floatVal = parseFloat(value); | |
| if (isNaN(floatVal)) { | |
| return value; | |
| } else { | |
| let formattedValue = Intl.NumberFormat(getLocale(), { | |
| style: "decimal", | |
| minimumFractionDigits: decimals, | |
| maximumFractionDigits: decimals, | |
| notation: notation, | |
| }).format(floatVal); | |
| if (!thousandSeparator) { | |
| formattedValue = formattedValue.replaceAll( | |
| getLocaleThousandSeparator(), | |
| "", | |
| ); | |
| } | |
| return currency?.id + " " + formattedValue; | |
| } | |
| } catch (e) { | |
| Sentry.captureException(e); | |
| return value; | |
| } | |
| } else { | |
| return value; | |
| } | |
| }, [value, decimals, currencyCode, notation, thousandSeparator, columnType]); | |
| if (isCellEditMode) { | |
| const [inputType, inputHTMLType]: [InputTypes, "TEXT" | "NUMBER"] = (() => { | |
| switch (columnType) { | |
| case ColumnTypes.NUMBER: | |
| return [InputTypes.NUMBER, "NUMBER"]; | |
| case ColumnTypes.CURRENCY: | |
| return [InputTypes.CURRENCY, "NUMBER"]; | |
| default: | |
| return [InputTypes.TEXT, "TEXT"]; | |
| } | |
| })(); | |
| const additionalProps: Record<string, unknown> = {}; | |
| if (inputType === InputTypes.CURRENCY) { | |
| additionalProps.inputHTMLType = "NUMBER"; | |
| additionalProps.leftIcon = ( | |
| <CurrencyTypeDropdown | |
| accentColor={accentColor} | |
| allowCurrencyChange={false} | |
| options={CurrencyDropdownOptions} | |
| selected={currencyCode} | |
| widgetId={widgetId} | |
| /> | |
| ); | |
| additionalProps.shouldUseLocale = true; | |
| additionalProps.decimals = decimals; | |
| } | |
| editor = ( | |
| <InlineCellEditor | |
| accentColor={accentColor} | |
| additionalProps={additionalProps} | |
| allowCellWrapping={allowCellWrapping} | |
| autoFocus={!isNewRow} | |
| compactMode={compactMode} | |
| inputHTMLType={inputHTMLType} | |
| inputType={inputType} | |
| isEditableCellValid={isEditableCellValid} | |
| multiline={isMultiline} | |
| onChange={editEvents.onChange} | |
| onDiscard={editEvents.onDiscard} | |
| onSave={editEvents.onSave} | |
| paddedInput={isNewRow} | |
| textSize={textSize} | |
| validationErrorMessage={validationErrorMessage} | |
| value={value} | |
| verticalAlignment={verticalAlignment} | |
| widgetId={widgetId} | |
| /> | |
| ); | |
| } | |
| return ( | |
| <Container | |
| cellBackground={cellBackground} | |
| className="t--table-text-cell" | |
| isCellEditMode={isCellEditMode} | |
| verticalAlignment={verticalAlignment} | |
| <Text | |
| color={cellColor === "default" ? undefined : cellColor} | |
| isBold={isBold} | |
| isItalic={isItalic} | |
| lineClamp={lineClamp} | |
| title={value} | |
| variant="body" | |
| > | |
| <BasicCell | |
| accentColor={accentColor} | |
| allowCellWrapping={allowCellWrapping} | |
| cellBackground={cellBackground} | |
| columnType={columnType} | |
| compactMode={compactMode} | |
| disabledEditIcon={disabledEditIcon} | |
| disabledEditIconMessage={disabledEditIconMessage} | |
| fontStyle={fontStyle} | |
| hasUnsavedChanges={hasUnsavedChanges} | |
| horizontalAlignment={horizontalAlignment} | |
| isCellDisabled={isCellDisabled} | |
| isCellEditMode={isCellEditMode} | |
| isCellEditable={isCellEditable} | |
| isCellVisible={isCellVisible} | |
| isHidden={isHidden} | |
| onEdit={editEvents.onEdit} | |
| ref={contentRef} | |
| tableWidth={tableWidth} | |
| textColor={textColor} | |
| textSize={textSize} | |
| url={columnType === ColumnTypes.URL ? props.value : null} | |
| value={formattedValue} | |
| verticalAlignment={verticalAlignment} | |
| /> | |
| {editor} | |
| </Container> | |
| {value} | |
| </Text> | |
| function resolveTextProps({ cellColor, isBold, isItalic, allowCellWrapping }) { | |
| return { | |
| color: cellColor === "default" ? undefined : cellColor, | |
| isBold, | |
| isItalic, | |
| lineClamp: allowCellWrapping ? undefined : 1, | |
| }; | |
| } | |
| function PlainTextCell(props: PlainTextCellProps & BaseCellComponentProps) { | |
| const { value } = props; | |
| const textProps = resolveTextProps(props); | |
| return ( | |
| <Text | |
| {...textProps} | |
| title={value} | |
| variant="body" | |
| > | |
| {value} | |
| </Text> | |
| ); | |
| } |
| <Checkbox | ||
| isDisabled={!!disabledCheckbox || !isCellEditable} | ||
| isLoading={false} | ||
| isRequired={false} | ||
| label="" | ||
| labelPosition={LabelPosition.Auto} | ||
| onCheckChange={() => onChange()} | ||
| widgetId={""} | ||
| isSelected={value} | ||
| onChange={() => onChange()} |
There was a problem hiding this comment.
The onChange handler is wrapped in an unnecessary arrow function, which could lead to performance issues due to re-rendering.
- onChange={() => onChange()}
+ onChange={onChange}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.
| <Checkbox | |
| isDisabled={!!disabledCheckbox || !isCellEditable} | |
| isLoading={false} | |
| isRequired={false} | |
| label="" | |
| labelPosition={LabelPosition.Auto} | |
| onCheckChange={() => onChange()} | |
| widgetId={""} | |
| isSelected={value} | |
| onChange={() => onChange()} | |
| <Checkbox | |
| isDisabled={!!disabledCheckbox || !isCellEditable} | |
| isRequired={false} | |
| isSelected={value} | |
| onChange={onChange} |
There was a problem hiding this comment.
@jsartisan This change request makes sense.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -67,7 +67,7 @@ export class LightModeTheme implements ColorModeTheme { | |||
| bgNegative: this.bgNegative.to("sRGB").toString(), | |||
| bgNegativeHover: this.bgNegativeHover.to("sRGB").toString(), | |||
| bgNegativeActive: this.bgNegativeActive.to("sRGB").toString(), | |||
| bgNegativeSubtle: this.bgNegativeActive.to("sRGB").toString(), | |||
| bgNegativeSubtle: this.bgNegativeSubtle.to("sRGB").toString(), | |||
There was a problem hiding this comment.
This was a typo in the LightModeTheme. bgNegativeSubtle was using the function of bgNegativeActive
| import type { LinkProps } from "./types"; | ||
| import styles from "./styles.module.css"; | ||
|
|
||
| export function Link(props: LinkProps) { |
There was a problem hiding this comment.
We have to use Link from react-aria-components. Also we need to make story here.
| @@ -29,7 +29,7 @@ export interface TextProps { | |||
| /** Sets the horizontal alignment of the inline-level content inside a block element or table-cell box. See [MDN](https://developer.mozilla.org/en-US/docs/Web/CSS/text-align). | |||
| * @default left | |||
| */ | |||
| textAlign?: "left" | "center" | "right"; | |||
| textAlign?: "start" | "center" | "end" | "left" | "right"; | |||
There was a problem hiding this comment.
Do we still need left and right here?
There was a problem hiding this comment.
I think we can remove it. Let me remove them.
| /> | ||
| </EdtiableCheckboxWrapper> | ||
| )} | ||
| {this.props.widgetProperties.type !== "WDS_TABLE_WIDGET" && |
| <Flex | ||
| alignItems="center" | ||
| gap="spacing-1" | ||
| style={{ pointerEvents: "none" }} |
There was a problem hiding this comment.
Why pointerEvents here is necessary?
There was a problem hiding this comment.
I don't remember why i added it. But seems like this is not necessary at all.
| @@ -1,3 +1,6 @@ | |||
| /* TODO(pawan): can we access css module variable from packages/widgets */ | |||
| @value colors: accent, neutral, negative, warning, positive; | |||
There was a problem hiding this comment.
I believe this should work like this — @import "@design-system/widgets/src/shared/colors/colors.module.css";
There was a problem hiding this comment.
Awesome! Thanks for this
| customJSControl: "TABLE_COMPUTE_VALUE", | ||
| helpText: "Sets the variant of the button", | ||
| options: Object.values(BUTTON_VARIANTS).map((variant) => ({ | ||
| label: capitalize(variant), |
There was a problem hiding this comment.
Why we need capitalize here? Can we make options compatible?
There was a problem hiding this comment.
Capitalizing for decorative purposes to be shown in the property pane.
We can capitalize the option directly on the constant object like this:
export const BUTTON_VARIANTS = {
filled: "Filled",
outlined: "Outlined",
ghost: "Ghost",
} as const;What do you think?
There was a problem hiding this comment.
Yes, I think it should work. I really don't like unreasonable mutation of types, data, etc.
| ) { | ||
| let text; | ||
| function PlainTextCell(props: PlainTextCellProps & BaseCellComponentProps) { | ||
| const { allowCellWrapping, cellColor, isBold, isItalic, value } = props; |
There was a problem hiding this comment.
Where do we use allowCellWrapping in UI? Do we really need this?
There was a problem hiding this comment.
Cell wrapping is this feature where we allow multi-line row
So, Cell wrapping true means allowing multiline otherwise we use lineClamp = 1
CleanShot.2024-05-14.at.11.29.07.mp4
There was a problem hiding this comment.
Let me know if don't need it.
There was a problem hiding this comment.
I'll check with Taras and let you know the result.
/ok-to-test tags="@tag.Widget"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9056956210
Commit: 618ef03
Cypress dashboard url: Click here!