Skip to content

TESTING EXTERNAL SCRIPT: external merge request from Contributor #37791

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.

Sign up for GitHub

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

Closed
jacquesikot wants to merge 5 commits into from
Closed

TESTING EXTERNAL SCRIPT: external merge request from Contributor #37791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2669051
feat: alt text in image widget
benjaminParisel Oct 31, 2024
243f6a3
feat(image): alt text in image
Nov 12, 2024
3a96e40
feat(image): alt text in image
Nov 13, 2024
20a37de
feat(image): alt text in image
Nov 13, 2024
f9c6707
Merge remote-tracking branch 'external-vhemery/feat/image_alt' into c…
jacquesikot Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,20 @@ describe(
200,
);
cy.EvaluateCurrentValue(this.dataSet.base64image.withPrefix);
cy.testJsontext("alternativetext", this.dataSet.base64image.altText);
cy.wait("@updateLayout").should(
Copy link
Contributor

@sagar-qa007 sagar-qa007 Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kindly remove waits.

"have.nested.property",
"response.body.responseMeta.status",
200,
);
Comment on lines +24 to +29
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Replace cy.wait with better assertions

Instead of using cy.wait, consider using Cypress's built-in retry-ability with assertions that wait for the expected state.

  cy.testJsontext("alternativetext", this.dataSet.base64image.altText);
- cy.wait("@updateLayout").should(
-   "have.nested.property",
-   "response.body.responseMeta.status",
-   200,
- );
+ cy.get("@updateLayout").should((response) => {
+   expect(response.response.body.responseMeta.status).to.equal(200);
+ });
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
cy.testJsontext("alternativetext", this.dataSet.base64image.altText);
cy.wait("@updateLayout").should(
"have.nested.property",
"response.body.responseMeta.status",
200,
);
cy.testJsontext("alternativetext", this.dataSet.base64image.altText);
cy.get("@updateLayout").should((response) => {
expect(response.response.body.responseMeta.status).to.equal(200);
});

cy.EvaluateCurrentValue(this.dataSet.base64image.altText);

cy.get(viewWidgetsPage.imageinner)
.invoke("attr", "src")
.should("contain", this.dataSet.base64image.withPrefix);
cy.get(viewWidgetsPage.imageinner)
.invoke("attr", "alt")
.should("contain", this.dataSet.base64image.altText);
cy.closePropertyPane();
});
},
Expand Down
7 changes: 7 additions & 0 deletions app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@ describe(
cy.get(viewWidgetsPage.imageinner)
.invoke("attr", "src")
.should("contain", this.dataSet.validateImage);
/**
* @param{TEXT} Alternative text
*/
cy.testCodeMirrorLast(this.dataSet.NewImageAltText);
cy.get(viewWidgetsPage.imageinner)
.invoke("attr", "alt")
.should("contain", this.dataSet.validateImageAltText);
Comment on lines +41 to +47
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Alt text validation looks good, but remove cy.wait usage

The alt text validation is well implemented using the correct selector and fixture data. However, there's a cy.wait(1000) earlier in the test that should be replaced with a proper assertion.

Replace the wait with an assertion:

- cy.wait(1000);
+ cy.get(viewWidgetsPage.imageinner).should('exist');
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* @param{TEXT} Alternative text
*/
cy.testCodeMirrorLast(this.dataSet.NewImageAltText);
cy.get(viewWidgetsPage.imageinner)
.invoke("attr", "alt")
.should("contain", this.dataSet.validateImageAltText);
/**
* @param{TEXT} Alternative text
*/
cy.get(viewWidgetsPage.imageinner).should('exist');
cy.testCodeMirrorLast(this.dataSet.NewImageAltText);
cy.get(viewWidgetsPage.imageinner)
.invoke("attr", "alt")
.should("contain", this.dataSet.validateImageAltText);

cy.closePropertyPane();
});

Expand Down
5 changes: 4 additions & 1 deletion app/client/cypress/fixtures/TestDataSet1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,11 @@
"multiSelectName": "MultiSelect1",
"defaultimage": "https://i0.wp.com/www.heyuguys.com/images/2016/04/The-Joker.png?fit=1920%2C960",
"NewImage": "https://cdn.dribbble.com/users/1787323/screenshots/4563995/dribbbe_hammer-01.png",
"NewImageAltText": "Thor's hammer planted into the ground",
"base64image": {
"withoutPrefix": "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAAAAAA6fptVAAAACklEQVR4nGNiAAAABgADNjd8qAAAAABJRU5ErkJggg==",
"withPrefix": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAAAAAA6fptVAAAACklEQVR4nGNiAAAABgADNjd8qAAAAABJRU5ErkJggg=="
"withPrefix": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAAAAAA6fptVAAAACklEQVR4nGNiAAAABgADNjd8qAAAAABJRU5ErkJggg==",
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix the trailing comma in base64 string.

The base64 string in withPrefix field contains a trailing comma which could cause parsing issues.

Apply this diff to fix the trailing comma:

-    "withPrefix": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAAAAAA6fptVAAAACklEQVR4nGNiAAAABgADNjd8qAAAAABJRU5ErkJggg==",
+    "withPrefix": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAAAAAA6fptVAAAACklEQVR4nGNiAAAABgADNjd8qAAAAABJRU5ErkJggg=="
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"withPrefix": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAAAAAA6fptVAAAACklEQVR4nGNiAAAABgADNjd8qAAAAABJRU5ErkJggg==",
"withPrefix": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAAAAAA6fptVAAAACklEQVR4nGNiAAAABgADNjd8qAAAAABJRU5ErkJggg=="

"altText": "A single pixel"
},
"textfun": "{{Table1.selectedRow.userName}}",
"textfunID": "{{Table1.selectedRow.id}}",
Expand All @@ -118,6 +120,7 @@
"RichTexteditorBody": "Here is the text area to edit html",
"userApi": "http://host.docker.internal:5001/v1",
"validateImage": "https://cdn.dribbble.com/users/1787323/screenshots/4563995/dribbbe_hammer-01.png",
"validateImageAltText": "Thor's hammer planted into the ground",
"defaultdata": "TestData",
"label": "one",
"rgbValue": "rgb(255, 0, 0)",
Expand Down
3 changes: 2 additions & 1 deletion app/client/src/widgets/ImageWidget/component/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ class ImageComponent extends React.Component<
>
{/* Used for running onImageError and onImageLoad Functions since Background Image doesn't have the functionality */}
<img
alt={this.props.widgetName}
alt={this.props.alt || this.props.widgetName || "Image"}
onError={this.onImageError}
onLoad={this.onImageLoad}
src={this.props.imageUrl || this.props.defaultImageUrl}
Expand Down Expand Up @@ -410,6 +410,7 @@ export interface ImageComponentProps extends ComponentProps {
onClick?: (event: React.MouseEvent<HTMLElement>) => void;
borderRadius: string;
boxShadow?: string;
alt?: string;
}

export default ImageComponent;
19 changes: 19 additions & 0 deletions app/client/src/widgets/ImageWidget/widget/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,23 @@ class ImageWidget extends BaseWidget<ImageWidgetProps, WidgetState> {
isTriggerProperty: false,
validation: { type: ValidationTypes.IMAGE_URL },
},
{
helpText: "Sets alternative text for the image",
propertyName: "alt",
label: "Alternative text",
controlType: "INPUT_TEXT",
placeholderText: "Alternative text",
isBindProperty: true,
defaultValue: "",
isTriggerProperty: false,
validation: {
type: ValidationTypes.TEXT,
params: {
required: true,
maxLength: 125
}
},
},
Comment on lines +136 to +152
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove required validation for alt property

The alt property should remain optional as there are fallback values when users don't provide alternative text.

Apply this diff to fix the validation:

             validation: { 
               type: ValidationTypes.TEXT,
               params: {
-                required: true,
                 maxLength: 125
               }
             },
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{
helpText: "Sets alternative text for the image",
propertyName: "alt",
label: "Alternative text",
controlType: "INPUT_TEXT",
placeholderText: "Alternative text",
isBindProperty: true,
defaultValue: "",
isTriggerProperty: false,
validation: {
type: ValidationTypes.TEXT,
params: {
required: true,
maxLength: 125
}
},
},
{
helpText: "Sets alternative text for the image",
propertyName: "alt",
label: "Alternative text",
controlType: "INPUT_TEXT",
placeholderText: "Alternative text",
isBindProperty: true,
defaultValue: "",
isTriggerProperty: false,
validation: {
type: ValidationTypes.TEXT,
params: {
maxLength: 125
}
},
},

],
},
{
Expand Down Expand Up @@ -328,6 +345,7 @@ class ImageWidget extends BaseWidget<ImageWidgetProps, WidgetState> {
disableDrag={(disable: boolean) => {
this.disableDrag(disable);
}}
alt={this.props.alt ? this.props.alt : undefined}
enableDownload={this.props.enableDownload}
enableRotation={this.props.enableRotation}
imageUrl={this.props.image}
Expand Down Expand Up @@ -368,6 +386,7 @@ export interface ImageWidgetProps extends WidgetProps {
onClick?: string;
borderRadius: string;
boxShadow?: string;
alt?: string;
}

export default ImageWidget;