Skip to content

feat: alt text in image widget #37371

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
vhemery wants to merge 4 commits into from
Closed

feat: alt text in image widget #37371

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 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
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(
"have.nested.property",
"response.body.responseMeta.status",
200,
);
cy.EvaluateCurrentValue(this.dataSet.base64image.altText);
Comment on lines +24 to +30
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024 •
edited
Loading

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 alternatives

The test implementation should avoid using cy.wait as per our guidelines. Consider using Cypress's built-in retry-ability with assertions instead.

  cy.testJsontext("alternativetext", this.dataSet.base64image.altText);
- cy.wait("@updateLayout").should(
-   "have.nested.property",
-   "response.body.responseMeta.status",
-   200,
- );
+ cy.get("@updateLayout").should("have.nested.property", "response.body.responseMeta.status", 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.EvaluateCurrentValue(this.dataSet.base64image.altText);
cy.testJsontext("alternativetext", this.dataSet.base64image.altText);
cy.get("@updateLayout").should("have.nested.property", "response.body.responseMeta.status", 200);
cy.EvaluateCurrentValue(this.dataSet.base64image.altText);

Copy link
Author

@vhemery vhemery Nov 13, 2024

Choose a reason for hiding this comment

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

Consistent with "image" update.

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

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

@vhemery, understood. Maintaining consistency with the "image" update makes sense. Thank you for the clarification.

🐰😔


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);
Comment on lines +35 to +37
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use data- attributes for selectors*

Replace the usage of viewWidgetsPage.imageinner with a data-* attribute selector for better test stability.

- cy.get(viewWidgetsPage.imageinner)
+ cy.get('[data-testid="image-widget"]')

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Author

@vhemery vhemery Nov 13, 2024

Choose a reason for hiding this comment

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

Consistent with "src" check.

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

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

@vhemery, understood. Maintaining consistency with the "src" check makes sense. Thanks for clarifying.

✏️ Learnings added 
Learnt from: vhemery
PR: appsmithorg/appsmith#37371
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js:35-37
Timestamp: 2024-11-13T09:11:36.663Z
Learning: In the file `app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js`, when writing Cypress end-to-end tests for the Image Widget in JavaScript, use `viewWidgetsPage.imageinner` as the selector when asserting 'src' and 'alt' attributes to maintain consistency.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

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 +44 to +47
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor test implementation to follow Cypress best practices.

The current implementation needs improvements to align with our coding guidelines:

-      cy.testCodeMirrorLast(this.dataSet.NewImageAltText);
-      cy.get(viewWidgetsPage.imageinner)
-        .invoke("attr", "alt")
-        .should("contain", this.dataSet.validateImageAltText);
+      const expectedAltText = this.dataSet.validateImageAltText;
+      cy.testCodeMirrorLast(this.dataSet.NewImageAltText);
+      cy.get('[data-testid="styledImage"]')
+        .should("be.visible")
+        .and("have.attr", "alt")
+        .and("contain", expectedAltText);

Changes:

  1. Use data-testid attribute instead of viewWidgetsPage.imageinner
  2. Add multiple assertions as per guidelines
  3. Store expected value in a variable to avoid string literals in assertions
📝 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.testCodeMirrorLast(this.dataSet.NewImageAltText);
cy.get(viewWidgetsPage.imageinner)
.invoke("attr", "alt")
.should("contain", this.dataSet.validateImageAltText);
const expectedAltText = this.dataSet.validateImageAltText;
cy.testCodeMirrorLast(this.dataSet.NewImageAltText);
cy.get('[data-testid="styledImage"]')
.should("be.visible")
.and("have.attr", "alt")
.and("contain", expectedAltText);

Copy link
Author

@vhemery vhemery Nov 13, 2024

Choose a reason for hiding this comment

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

If committed anywhere, this suggestion should be on "src" check...

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

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

@vhemery, understood. Thank you for pointing that out.

✏️ Learnings added 
Learnt from: vhemery
PR: appsmithorg/appsmith#37371
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_spec.js:44-47
Timestamp: 2024-11-13T09:07:54.787Z
Learning: When reviewing code in `app/client/cypress/**` paths, ensure that suggestions are appropriate for test code and not for `src` code. Avoid applying source code checks to test code unless relevant.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

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==",
"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;
12 changes: 12 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,16 @@ 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: false,
isTriggerProperty: false,
validation: { type: ValidationTypes.TEXT },
},
],
},
{
Expand Down Expand Up @@ -328,6 +338,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 +379,7 @@ export interface ImageWidgetProps extends WidgetProps {
onClick?: string;
borderRadius: string;
boxShadow?: string;
alt?: string;
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Make alt text required and add JSDoc

For better accessibility compliance, alt text should be a required property with proper documentation.

-alt?: string;
+/** Alternative text for accessibility. Required for WCAG compliance. */
+alt: string;
📝 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
alt?: string;
/** Alternative text for accessibility. Required for WCAG compliance. */
alt: string;

Copy link
Author

@vhemery vhemery Nov 13, 2024

Choose a reason for hiding this comment

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

alt should not be required as we have fallback values in case the user does not fill it.

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

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

@vhemery, understood. Keeping the alt attribute optional makes sense given the fallback values when it's not provided.

✏️ Learnings added 
Learnt from: vhemery
PR: appsmithorg/appsmith#37371
File: app/client/src/widgets/ImageWidget/widget/index.tsx:382-382
Timestamp: 2024-11-13T09:13:10.854Z
Learning: In the `ImageWidget` component (`app/client/src/widgets/ImageWidget/widget/index.tsx`), the `alt` property should remain optional (`alt?: string;`) because there are fallback values when the user does not provide alternative text.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

}

export default ImageWidget;