feat: alt text in image widget #37371
Conversation
Add the alt attribute as a new widget property Fixes [appsmithorg#13383](appsmithorg#13383)
Add tests. Fixes [appsmithorg#13383](appsmithorg#13383)
WalkthroughThe pull request introduces enhancements to the Image Widget functionality, specifically focusing on adding and validating alternative text (alt text) for images. Modifications include updates to test cases for validating alt text, updates to JSON test data, and changes to the ImageComponent and ImageWidget classes to support the new alt text property. These changes aim to improve accessibility and ensure proper functionality of the Image Widget. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@vhemery has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js (2)
Line range hint
41-43: Remove afterEach blockThe afterEach block should be removed as per our guidelines. If cleanup is necessary, consider using API calls or more direct approaches.
- afterEach(() => { - // put your clean up code if any - });
Line range hint
1-43: Additional test improvements neededSeveral improvements are needed to align with our testing guidelines:
- Login/logout operations should use API calls (LoginFromAPI/LogOutviaAPI)
- Consider combining multiple assertions for better test efficiency
- Move selector strings to locator variables
app/client/src/widgets/ImageWidget/widget/index.tsx (1)
341-341: Simplify alt text prop passingThe conditional check can be simplified since undefined is already the desired fallback value.
-alt={this.props.alt ? this.props.alt : undefined} +alt={this.props.alt}app/client/cypress/fixtures/TestDataSet1.json (2)
98-99: Consider standardizing alt text property nameThe property is named
altTexthere butNewImageAltTextandvalidateImageAltTextelsewhere in the file.Consider using consistent property naming across all image objects:
- "altText": "A single pixel" + "imageAltText": "A single pixel"
123-123: Avoid duplicate alt text descriptionsThe same alt text is used for both
NewImageAltTextandvalidateImageAltText. While this might be intentional for testing, consider using distinct descriptions to better test different scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js(1 hunks)app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_spec.js(1 hunks)app/client/cypress/fixtures/TestDataSet1.json(2 hunks)app/client/src/widgets/ImageWidget/component/index.tsx(2 hunks)app/client/src/widgets/ImageWidget/widget/index.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_spec.js (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/fixtures/TestDataSet1.json (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (4)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_spec.js (1)
41-43: LGTM! Documentation is clear and consistent.
app/client/src/widgets/ImageWidget/widget/index.tsx (1)
341-341: Verify alt text implementation in ImageComponent and usage
Let's ensure the alt text is properly implemented in the ImageComponent and handled in existing image instances.
app/client/src/widgets/ImageWidget/component/index.tsx (1)
413-413: LGTM! Verify accessibility implementation
The alt property is correctly defined as an optional string.
Let's verify the accessibility implementation:
✅ Verification successful
Alt property implementation is complete and correct
The alt property is:
- Properly defined in both ImageWidgetProps and ImageComponentProps interfaces
- Correctly passed through the component hierarchy
- Properly rendered in the img element with appropriate fallback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if alt property is properly used in test files
rg -g "*.{tsx,ts}" "alt.*=.*" app/client/src/widgets/ImageWidget/
Length of output: 273
Script:
#!/bin/bash
# Check if alt property is properly defined in widget props
ast-grep --pattern 'interface ImageWidgetProps {
$$$
alt?: string;
$$$
}'
# Check if alt property is properly passed through component hierarchy
rg -g "*.{tsx,ts}" "alt.*:.*" app/client/src/widgets/ImageWidget/
Length of output: 398
app/client/cypress/fixtures/TestDataSet1.json (1)
95-95: LGTM: Alt text property added for NewImage
The alt text description is clear and descriptive, aligning well with accessibility best practices.
| 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); |
There was a problem hiding this comment.
🛠️ 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.
| 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); |
There was a problem hiding this comment.
Consistent with "image" update.
There was a problem hiding this comment.
@vhemery, understood. Maintaining consistency with the "image" update makes sense. Thank you for the clarification.
🐰😔
| cy.get(viewWidgetsPage.imageinner) | ||
| .invoke("attr", "alt") | ||
| .should("contain", this.dataSet.base64image.altText); |
There was a problem hiding this comment.
🛠️ 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.
There was a problem hiding this comment.
Consistent with "src" check.
There was a problem hiding this comment.
@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.testCodeMirrorLast(this.dataSet.NewImageAltText); | ||
| cy.get(viewWidgetsPage.imageinner) | ||
| .invoke("attr", "alt") | ||
| .should("contain", this.dataSet.validateImageAltText); |
There was a problem hiding this comment.
🛠️ 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:
- Use data-testid attribute instead of viewWidgetsPage.imageinner
- Add multiple assertions as per guidelines
- 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.
| 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); |
There was a problem hiding this comment.
If committed anywhere, this suggestion should be on "src" check...
There was a problem hiding this comment.
@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.
| @@ -368,6 +379,7 @@ export interface ImageWidgetProps extends WidgetProps { | |||
| onClick?: string; | |||
| borderRadius: string; | |||
| boxShadow?: string; | |||
| alt?: string; | |||
There was a problem hiding this comment.
🛠️ 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.
| alt?: string; | |
| /** Alternative text for accessibility. Required for WCAG compliance. */ | |
| alt: string; |
There was a problem hiding this comment.
alt should not be required as we have fallback values in case the user does not fill it.
There was a problem hiding this comment.
@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.
Use fallback values for alt property Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Dynamic binding and length validation. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
🆙 This PR is awaiting review by a code owner. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Description
Add the alt attribute as a new widget property for the image widget.
Fixes #13383
Automation
/test Image
/ok-to-test tags="Image"
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation