feature/BA-2628-add-aria-label-and-alt-text-to-profile-images-and-inp… #281
Conversation
…uts-for-tests-component-identification
lbjunq
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughAdded accessibility attributes ( Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx(5 hunks)packages/design-system/components/web/avatars/AvatarWithPlaceholder/index.tsx(1 hunks)packages/design-system/components/web/avatars/CircledAvatar/index.tsx(1 hunks)packages/design-system/components/web/buttons/FileUploadButton/index.tsx(2 hunks)packages/design-system/components/web/buttons/FileUploadButton/types.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
packages/design-system/components/web/avatars/CircledAvatar/index.tsx (1)
Learnt from: davidbermudez-tsl
PR: #209
File: packages/components/modules/content-feed/web/ContentFeed/types.ts:1-1
Timestamp: 2025-04-09T22:06:40.026Z
Learning: The empty ContentFeedProps interface in packages/components/modules/content-feed/web/ContentFeed/types.ts will be populated with properties in a future PR rather than being converted to a type alias.
packages/design-system/components/web/avatars/AvatarWithPlaceholder/index.tsx (1)
Learnt from: davidbermudez-tsl
PR: #209
File: packages/components/modules/content-feed/web/ContentFeed/types.ts:1-1
Timestamp: 2025-04-09T22:06:40.026Z
Learning: The empty ContentFeedProps interface in packages/components/modules/content-feed/web/ContentFeed/types.ts will be populated with properties in a future PR rather than being converted to a type alias.
packages/design-system/components/web/buttons/FileUploadButton/types.ts (1)
Learnt from: davidbermudez-tsl
PR: #209
File: packages/components/modules/content-feed/web/ContentFeed/types.ts:1-1
Timestamp: 2025-04-09T22:06:40.026Z
Learning: The empty ContentFeedProps interface in packages/components/modules/content-feed/web/ContentFeed/types.ts will be populated with properties in a future PR rather than being converted to a type alias.
🧬 Code Graph Analysis (1)
packages/design-system/components/web/buttons/FileUploadButton/types.ts (1)
packages/utils/types/form.ts (1)
ControlProps(5-8)
🪛 GitHub Check: Build and Lint Packages
packages/design-system/components/web/buttons/FileUploadButton/types.ts
[failure] 3-3:
'InputProps' is defined but never used. Allowed unused vars must match /^_/u
🪛 GitHub Actions: Main Workflow
packages/design-system/components/web/buttons/FileUploadButton/types.ts
[error] 3-3: ESLint: 'InputProps' is defined but never used. Allowed unused vars must match /^_/u (@typescript-eslint/no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/design-system/components/web/avatars/CircledAvatar/index.tsx (1)
14-14: LGTM! Clean accessibility enhancement.The addition of the
altprop and its proper forwarding to theAvatarWithPlaceholdercomponent is a clean accessibility improvement. The implementation correctly follows React patterns for prop passing.Also applies to: 22-22
packages/design-system/components/web/buttons/FileUploadButton/index.tsx (1)
15-15: LGTM! Proper accessibility implementation.The extraction and forwarding of the
aria-labelattribute to the hidden file input is correctly implemented. This enhancement will improve screen reader support for file upload functionality.Also applies to: 52-52
packages/design-system/components/web/avatars/AvatarWithPlaceholder/index.tsx (1)
11-11: LGTM! Consistent accessibility prop forwarding.The addition of the
altprop and its forwarding toAvatarStyledmaintains consistency with the accessibility improvements across the avatar component chain. The implementation is clean and correct.Also applies to: 14-14
packages/design-system/components/web/buttons/FileUploadButton/types.ts (1)
24-26: LGTM! Proper type extension for accessibility.The addition of the optional
'aria-label'property toFileUploadButtonPropscorrectly supports the accessibility enhancements in the component implementation.packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (1)
153-153: Excellent accessibility improvements!The addition of descriptive
altandaria-labelattributes across avatar, banner, and button elements significantly improves screen reader support. The labels are appropriately descriptive and follow accessibility best practices.Also applies to: 169-169, 178-178, 249-249, 259-259
packages/design-system/components/web/buttons/FileUploadButton/types.ts
Outdated
Show resolved
Hide resolved
mathieubouhelier
requested review from
mathieubouhelier
and removed request for
mathieubouhelier
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/components/modules/profiles/web/ProfileSettingsComponent/__tests__/__mocks__/requests.ts (4)
5-5: Align mock IDs with the test query’s hardcoded node idThe test query fetches node(id: "test-id"), but mocks use "profile-123". Not harmful with a permissive test env, yet aligning avoids confusion and future brittleness if resolvers validate IDs.
Apply this diff:
- id: 'profile-123', + id: 'test-id', @@ - id: `profile-123`, + id: 'test-id', @@ - id: `profile-123`, + id: 'test-id', @@ - id: `profile-123`, + id: 'test-id', @@ - id: `profile-123`, + id: 'test-id', @@ - id: `profile-123`, + id: 'test-id', @@ - id: 'profile-123', + id: 'test-id',Also applies to: 30-30, 59-59, 88-88, 118-118, 147-147, 175-175
64-66: Prefer hermetic/local image URLs to avoid external flakinessHardcoding external CDN URLs in mocks can introduce intermittent failures (network hiccups, CORS). Since tests only need the URL string, prefer localhost or data: URLs.
Example diff for local URLs:
- url: 'https://nyc3.digitaloceanspaces.com/baseapp-production-storage/media/user-avatars/5/6/4/resized/50/50/185a04dfdaa512d218cf9b7a5097e3c9.png', + url: 'http://localhost:8000/media/mock/avatar.png', @@ - url: 'https://nyc3.digitaloceanspaces.com/baseapp-production-storage/media/user-avatars/5/6/4/resized/50/50/185a04dfdaa512d218cf9b7a5097e3c9.png', + url: 'http://localhost:8000/media/mock/banner.png', @@ - url: 'http://localhost:8000/media/profile_images/5dfe5729-1730-4fe6-b22a-a0f15f65a754.png.96x96_q85.png', + url: 'http://localhost:8000/media/mock/avatar.png', - url: 'http://localhost:8000/media/banner_images/test-banner.png', + url: 'http://localhost:8000/media/mock/banner.png',Also applies to: 125-127, 180-184
1-24: Reduce duplication with a small builder to improve maintainabilityThe seven mocks share a large common shape with small diffs. A builder/helper would reduce noise and ease future changes (e.g., add/remove fields).
Example:
const baseProfile = { __typename: 'Profile' as const, id: 'test-id', status: 'ACTIVE', name: 'John Doe', biography: 'John Doe is a software engineer at Google.', image: null as { url: string | null } | null, bannerImage: null as { url: string | null } | null, isFollowedByMe: false, followersCount: 0, followingCount: 0, canChange: true, urlPath: { path: '/johndoes' }, owner: { phoneNumber: '+15551234567' }, isBlockedByMe: false, } const makeData = (overrides: Partial<typeof baseProfile>) => ({ data: { target: { ...baseProfile, ...overrides } } }) export const profileSettingsMockData = makeData({}) export const profileSettingsTextUpdateData = makeData({ name: 'Jane Smith', biography: 'Jane Smith is a software engineer at Microsoft.', image: { url: null }, bannerImage: { url: null }, urlPath: { path: '/janesmith' }, owner: { phoneNumber: '+11234567890' }, }) // ...and so on for other variantsAlso applies to: 26-53, 55-82, 84-111, 113-140, 142-169, 171-198
8-8: Nit: avoid referencing real companies in test biosNot a blocker, but consider neutral placeholders (e.g., “ACME Corp”) to avoid unnecessary real-world references in public code.
Also applies to: 33-33, 62-62, 91-91, 120-120, 149-149, 178-178
packages/components/modules/profiles/web/ProfileSettingsComponent/__tests__/ProfileSettings.cy.tsx (1)
23-35: Stabilize selectors for file inputs and fallback imagesThe current selectors depend on DOM structure (
buttonparent ->input[type="file"]) and a broadimg, [title="Avatar Fallback"]. Introduce data-testid or more specific roles to reduce flakiness.Example:
- Add
data-testid="avatar-file-input"anddata-testid="banner-file-input"to the underlying inputs and usecy.findByTestId(...).- Use
cy.findByRole('img', { name: /avatar image/i })for images if roles are present.Also applies to: 39-47
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/components/cypress/fixtures/tsl-logo.pngis excluded by!**/*.png
📒 Files selected for processing (3)
packages/components/modules/profiles/web/ProfileSettingsComponent/__tests__/ProfileSettings.cy.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__tests__/__mocks__/requests.ts(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__tests__/__utils__/ProfileSettingsForTesting/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/components/modules/profiles/web/ProfileSettingsComponent/__tests__/ProfileSettings.cy.tsx (1)
packages/components/modules/profiles/web/ProfileSettingsComponent/__tests__/__mocks__/requests.ts (6)
profileSettingsMockData(1-24)profileSettingsTextUpdateData(26-53)profileSettingsImageUpdateData(55-82)profileSettingsImageRemoveData(84-111)profileSettingsBannerUpdateData(113-140)profileSettingsBannerRemoveData(142-169)
packages/components/modules/profiles/web/ProfileSettingsComponent/__tests__/__utils__/ProfileSettingsForTesting/index.tsx (1)
packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts (1)
ProfileSettingsComponentProps(3-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (4)
packages/components/modules/profiles/web/ProfileSettingsComponent/__tests__/__utils__/ProfileSettingsForTesting/index.tsx (1)
10-19: LGTM: minimal test-only query using @relay_test_operationThe test query/setup is clean and focused for the test harness.
packages/components/modules/profiles/web/ProfileSettingsComponent/__tests__/ProfileSettings.cy.tsx (3)
7-14: Remove unused import to satisfy lint rules
mockDataWithBanneris imported but not used in this file.Apply this diff:
import { - mockDataWithBanner, profileSettingsBannerRemoveData, profileSettingsBannerUpdateData, profileSettingsImageRemoveData, profileSettingsImageUpdateData, profileSettingsMockData, profileSettingsTextUpdateData, } from './__mocks__/requests'
50-75: Great coverage and realistic flowsSolid end-to-end coverage for render, validation, text updates, and image workflows. The test environment/wiring via queueOperationResolver and resolveMostRecentOperation looks robust.
Also applies to: 77-100, 102-133, 135-175, 177-215
3-3: Heads-up: Next internal import path can be brittle
next/dist/shared/lib/app-router-context.shared-runtimeis an internal path and can change across Next versions. If you hit issues in future Next upgrades, revisit this import.
...ges/components/modules/profiles/web/ProfileSettingsComponent/__tests__/__mocks__/requests.ts
Show resolved
Hide resolved
...rofiles/web/ProfileSettingsComponent/__tests__/__utils__/ProfileSettingsForTesting/index.tsx
Show resolved
Hide resolved
...rofiles/web/ProfileSettingsComponent/__tests__/__utils__/ProfileSettingsForTesting/index.tsx
Show resolved
Hide resolved
...es/components/modules/profiles/web/ProfileSettingsComponent/__tests__/ProfileSettings.cy.tsx
Show resolved
Hide resolved
...es/components/modules/profiles/web/ProfileSettingsComponent/__tests__/ProfileSettings.cy.tsx
Outdated
Show resolved
Hide resolved
|
lbjunq
deleted the
feature/BA-2628-add-aria-label-and-alt-text-to-profile-images-and-inputs-for-tests-component-identification
branch
✅ Multiple Profiles - Members Suggestion Dropdown
Description
Test: When the user accesses the profile page, they see the form with profile fields, avatar, and banner sections.
Expected Outcome:
The form fields (name, username, phone number, and bio) and buttons (upload, remove, save changes) are displayed.
Test: The user uploads a new avatar image, and it appears in the preview.
Expected Outcome:
The uploaded avatar is displayed in the preview, and the "Remove" button is enabled.
Test: The user clicks "Remove" to clear the avatar image.
Expected Outcome:
The avatar image is removed, and the default placeholder is displayed.
Test: The user uploads a new banner image, and it appears in the preview.
Expected Outcome:
The uploaded banner is displayed in the preview, and the "Remove" button is enabled.
Test: The user enters invalid data in the form and sees validation errors.
Expected Outcome:
The user sees error messages under the invalid fields.
Test: The user clicks "Save Changes," and the form is submitted successfully.
Expected Outcome:
The form data is saved, and the user sees a success toast.
Summary by CodeRabbit