-
Notifications
You must be signed in to change notification settings - Fork 9
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.
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
fix(docs): removed external links for images and used static #1451
Conversation
WalkthroughThis pull request implements changes across multiple components in the React project, focusing on updating image sources. It replaces hardcoded URLs with local asset imports for the Avatar, Card, InviteAgents, Picker, and Tag components. The modifications enhance the visual representation of these components in Storybook without altering their overall structure or functionality. Changes
Possibly related PRs
Suggested reviewers
🪧 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/react-components/src/components/Picker/Picker.stories.tsx (1)
5-5
: Consider extracting repeated image element into a shared componentThe same image markup is duplicated across multiple custom elements. Consider creating a reusable component:
+const CustomPickerImage = ({ isSelected }) => ( + <img + className={`image${isSelected ? ' selected' : ''}`} + src={image} + alt="Custom picker item" + /> +); // Usage: -<img className="image" src={image} /> +<CustomPickerImage /> -<img className="image selected" src={image} /> +<CustomPickerImage isSelected />Also applies to: 175-175, 184-184, 198-198, 207-207, 228-228, 237-237, 249-249, 258-258
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/react-components/src/stories/assets/avatar.jpg
is excluded by!**/*.jpg
packages/react-components/src/stories/assets/folder.svg
is excluded by!**/*.svg
📒 Files selected for processing (7)
packages/react-components/src/components/Avatar/Avatar.stories.tsx
(5 hunks)packages/react-components/src/components/Card/Card.module.scss
(0 hunks)packages/react-components/src/components/Card/Card.stories.tsx
(5 hunks)packages/react-components/src/components/InviteAgents/InviteAgents.stories.tsx
(2 hunks)packages/react-components/src/components/Picker/Picker.stories.tsx
(9 hunks)packages/react-components/src/components/Picker/constants.tsx
(2 hunks)packages/react-components/src/components/Tag/Tag.stories.tsx
(2 hunks)
💤 Files with no reviewable changes (1)
- packages/react-components/src/components/Card/Card.module.scss
🔇 Additional comments (4)
packages/react-components/src/components/Card/Card.stories.tsx (1)
6-6
: LGTM! Verify image asset existence
The placeholder URLs have been consistently replaced with the static image.
Also applies to: 28-28, 39-39, 51-51, 74-74
✅ Verification successful
Image asset verified successfully
The folder.svg
image exists at the expected location: packages/react-components/src/stories/assets/folder.svg
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the image asset exists
fd -t f "folder.svg" packages/react-components/src/stories/assets/
Length of output: 122
packages/react-components/src/components/InviteAgents/InviteAgents.stories.tsx (1)
4-5
: Verify the mixed avatar states
The changes maintain a mix of avatar states (empty, undefined, and image) which appears intentional for testing different scenarios.
Also applies to: 42-42, 49-49, 56-56
✅ Verification successful
Let me verify the usage of mixed avatar states in the code.
Mixed avatar states are correctly implemented for testing
The mockAgents array intentionally includes all avatar scenarios:
- Empty string avatar (Bob Smith)
- Undefined avatar (Alice Johnson)
- Image avatar (Alice Johnson 2, Bob Smith, Unknown Agents)
This variety ensures proper testing of avatar rendering in different states.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the avatar image exists
fd -t f "avatar.jpg" packages/react-components/src/stories/assets/
Length of output: 122
Script:
#!/bin/bash
# Check the content of InviteAgents.stories.tsx to verify mixed avatar states
rg -U "mockAgents.*?\]" packages/react-components/src/components/InviteAgents/InviteAgents.stories.tsx
Length of output: 104
Script:
#!/bin/bash
# Let's try a different approach to get the content
cat packages/react-components/src/components/InviteAgents/InviteAgents.stories.tsx
Length of output: 2564
packages/react-components/src/components/Picker/constants.tsx (1)
3-3
: Consider dedicated picker assets
While reusing avatar.jpg works, consider using a more contextually appropriate image for picker examples.
Also applies to: 87-87
packages/react-components/src/components/Tag/Tag.stories.tsx (1)
6-6
: LGTM: Image source updated correctly
The hardcoded URL has been properly replaced with a local image import.
Also applies to: 19-19
@@ -2,6 +2,7 @@ import * as React from 'react'; | |||
|
|||
import { Meta } from '@storybook/react'; | |||
|
|||
import image from '../../stories/assets/avatar.jpg'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Missing alt text in multiple Avatar components
Several Avatar components with type="image" are missing the alt attribute:
- Line with
withRim type="image" src={image}
- Line with
withRim status="available" type="image" src={image}
- Line with
type="image" src={image} size={size} status="available"
- Line with
type="image" src={image} size={size} withRim
- Line with
type="image" src="https://example.com/not-a-proper-image.png"
🔗 Analysis chain
Ensure image accessibility attributes are consistent
While the image source updates are correct, verify that all instances have proper alt text for accessibility.
Also applies to: 30-30, 102-102, 105-105, 115-115, 134-134
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for missing alt attributes in Avatar components
rg -l "Avatar.*type=\"image\"" | xargs rg "type=\"image\"" -A 2 -B 2
Length of output: 1044
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
Resolves: #{issue-number}
Description
removed external links for images and used static
Storybook
https://feature-external-images-remove--613a8e945a5665003a05113b.chromatic.com
Checklist
Obligatory:
Summary by CodeRabbit
New Features
Card
,Avatar
,Picker
, andTag
components in Storybook.Bug Fixes
Style