fix: normalized logos of sponsors #3169
Conversation
Signed-off-by: wck-iipi <[email protected]>
Wck-iipi
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
❌ Deploy Preview for asyncapi-website failed.Built without sensitive environment variables
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
Wck-iipi
changed the title
Wck-iipi
changed the title
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3169--asyncapi-website.netlify.app/ |
| * @param {string} props.className - Additional CSS class names for styling the image. | ||
| * @param {string} props.dataTestId - A string for identifying the element during testing. | ||
| */ | ||
| const ResponsiveImage: React.FC<ResponsiveImageProps> = ({ src, alt, className, dataTestId }) => { |
There was a problem hiding this comment.
Why to create separate component for Responsive images? We can add this property to component of respective category.
There was a problem hiding this comment.
HDI logo size is way too huge as compared to others, so probably we have to built a component that restricts the logo size with fixed dimension.
I thought that creating a component was required? Do you want me to do this without writing a new component or with?
Signed-off-by: wck-iipi <[email protected]>
|
@akshatnema I have made the changes can you please check this PR. Thanks. |
|
@coderabbitai summarize |
|
Tip For best results, initiate chat on the files or code changes. @akshatnema, here's the summary of the PR: Fix: Normalized Logos of Sponsors This pull request addresses issue #3158 by normalizing the logos of sponsors on the AsyncAPI website. The changes ensure that all sponsor logos are adjusted for consistent appearance, improving the overall look and feel of the sponsors section. |
|
@Wck-iipi Please resolve the conflicts in your PR. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes involve the introduction of a new Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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
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
🧹 Outside diff range and nitpick comments (4)
components/sponsors/ResponsiveImage.tsx (1)
19-23: Minor optimization: Simplify className application.The component implementation looks good overall. The use of
twMergeaddresses the previous comment about using tailwind-merge. However, we can slightly simplify the className application.Consider updating the img element as follows:
return <img src={src} alt={alt} className={classNameMerged} data-testid={dataTestId} />;This removes the unnecessary template literal, as
classNameMergedalready contains the merged classes.components/sponsors/GoldSponsors.tsx (1)
28-33: LGTM: ResponsiveImage component implemented correctly.The
<ResponsiveImage>component is correctly implemented, replacing the<img>tag. This change aligns with the PR objective of normalizing sponsor logos. Thesrc,alt,className, anddataTestIdprops are appropriately set.For consistency with React conventions, consider updating the
data-testidattribute on the parent<a>tag to also use camelCase (dataTestId):- data-testid='GoldSponsors-link' + dataTestId='GoldSponsors-link'components/sponsors/SilverSponsors.tsx (1)
Line range hint
17-37: LGTM: Overall implementation, with a minor suggestion for accessibilityThe changes to the
SilverSponsorscomponent are well-implemented and focused on the image rendering, which aligns with the PR objective. The overall structure and functionality of the component have been preserved.As a minor suggestion for improvement, consider adding an
aria-labelto the sponsor links to enhance accessibility. For example:<a key={index} href={sponsor.website} target='_blank' className='relative block w-2/3 p-4 text-center sm:w-1/2 md:w-1/3 lg:w-1/4' rel='noopener noreferrer' data-testid='SilverSponsors-link' + aria-label={`Visit ${sponsor.altText}'s website`} >This change would provide more context for screen reader users.
components/sponsors/Sponsors.tsx (1)
28-32: LGTM: ResponsiveImage component implemented correctly.The
<ResponsiveImage>component is correctly implemented, replacing the<img>tag. This change aligns with the PR objectives to normalize sponsor logos. The use ofdataTestIdfollows React naming conventions better than the previousdata-testidattribute.For consistency, consider renaming
dataTestIdtodata-testidin theResponsiveImagecomponent if it's meant to be used as a DOM attribute. If it's a custom prop, the current naming is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- components/sponsors/GoldSponsors.tsx (2 hunks)
- components/sponsors/ResponsiveImage.tsx (1 hunks)
- components/sponsors/SilverSponsors.tsx (2 hunks)
- components/sponsors/Sponsors.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (11)
components/sponsors/ResponsiveImage.tsx (4)
1-2: LGTM: Imports are correct and necessary.The imports for React and twMerge are appropriate for this component.
11-18: LGTM: Component documentation is clear and comprehensive.The documentation for the
ResponsiveImagecomponent is well-written, following JSDoc standards. It clearly describes the purpose of the component and each of its props.
25-25: LGTM: Export statement is correct.The default export of the
ResponsiveImagecomponent is appropriate and follows standard practices for React components.
1-25: Addressing past review comments.
Regarding the creation of a separate component for responsive images:
This implementation aligns with the original issue request (normalize the logos of sponsors #3158) to create a component that restricts logo sizes. It promotes reusability across different sponsor categories.The suggestion to use tailwind-merge has been successfully implemented in this component.
These changes effectively address the concerns raised in the previous review.
components/sponsors/GoldSponsors.tsx (2)
2-2: LGTM: ResponsiveImage import added correctly.The import statement for the new
ResponsiveImagecomponent is correctly added and necessary for the changes in the component's render method.
28-33: Verify ResponsiveImage behavior with various logo sizes.The implementation of the
ResponsiveImagecomponent looks good and addresses the issue of normalizing sponsor logos. However, it's important to ensure that the component handles different logo sizes correctly.To verify the behavior, please run the following script to check for any extreme variations in logo dimensions:
This script will help identify any logos that might still be disproportionately sized compared to others.
components/sponsors/SilverSponsors.tsx (3)
1-1: LGTM: New import statement for ResponsiveImageThe addition of the
ResponsiveImageimport aligns with the PR objective of normalizing sponsor logos. This change suggests a move towards a more standardized approach for rendering images across the application.
Line range hint
1-37: Summary: Changes align with PR objective, with minor suggestionsThe changes to the
SilverSponsorscomponent successfully implement theResponsiveImagecomponent, which should lead to more consistent rendering of sponsor logos as per the PR objective. The overall structure and functionality of the component have been preserved.Next steps:
- Verify the impact of changing
data-testidtodataTestIdon existing tests.- Consider implementing the suggested accessibility improvement by adding
aria-labelto sponsor links.- If these minor issues are addressed, the changes are ready for approval and merging.
Great work on improving the consistency of sponsor logo rendering!
28-32: Approve ResponsiveImage usage, but verify dataTestId propThe replacement of the
imgtag with theResponsiveImagecomponent is a good change that aligns with the PR objective of normalizing sponsor logos. The props are correctly passed from the sponsor object.However, the change from
data-testidtodataTestIdmight be a breaking change for existing tests.Please run the following script to check if there are any occurrences of
data-testidin test files that might need updating:If the script returns any results, consider updating those occurrences to match the new
dataTestIdprop or ensure that theResponsiveImagecomponent internally convertsdataTestIdtodata-testidfor backwards compatibility.components/sponsors/Sponsors.tsx (2)
1-1: LGTM: ResponsiveImage import added correctly.The import statement for
ResponsiveImageis correctly placed and uses a relative path, which is consistent with local component imports.
Line range hint
1-52: LGTM: Overall changes align with PR objectives.The modifications to the Sponsors component are focused and align well with the PR objectives to normalize sponsor logos. The overall structure and functionality of the component remain intact, with the primary change being the image rendering method.
To ensure the ResponsiveImage component is implemented correctly, please run the following verification script:
This script will help verify the correct implementation and usage of the ResponsiveImage component across sponsor-related files.
✅ Verification successful
LGTM: ResponsiveImage component is correctly implemented and utilized.
The ResponsiveImage component exists, its props are properly defined, and it is consistently imported across all sponsor-related components (
Sponsors.tsx,SilverSponsors.tsx, andGoldSponsors.tsx). These verifications confirm that the changes align with the PR objectives and maintain the integrity of the Sponsors component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of ResponsiveImage component # Test 1: Check if ResponsiveImage component exists echo "Checking for ResponsiveImage component..." ast-grep --lang typescript --pattern $'export default function ResponsiveImage($_) { $$$ }' # Test 2: Verify ResponsiveImage props echo "Verifying ResponsiveImage props..." ast-grep --lang typescript --pattern $'interface ResponsiveImageProps { src: string; alt: string; className?: string; dataTestId?: string; $$$ }' # Test 3: Check usage of ResponsiveImage in other sponsor-related components echo "Checking usage of ResponsiveImage in other components..." rg --type typescript 'import.*ResponsiveImage' components/sponsors/Length of output: 651
Script:
#!/bin/bash # Description: Re-verify the implementation of ResponsiveImage component with corrected rg command # Test 1: Check if ResponsiveImage component exists echo "Checking for ResponsiveImage component..." ast-grep --lang typescript --pattern $'export default function ResponsiveImage($_) { $$$ }' # Test 2: Verify ResponsiveImage props echo "Verifying ResponsiveImage props..." ast-grep --lang typescript --pattern $'interface ResponsiveImageProps { src: string; alt: string; className?: string; dataTestId?: string; $$$ }' # Test 3: Check usage of ResponsiveImage in other sponsor-related components echo "Checking usage of ResponsiveImage in other components..." rg 'import.*ResponsiveImage' components/sponsors/ --glob '*.ts' --glob '*.tsx'Length of output: 883
asyncapi-bot
requested review from
devilkiller-ag,
sambhavgupta0705,
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
|
@akshatnema I have resolved the conflict and made alt a compulsory attribute for accessibility purposes. Please review this PR. |
|
@Wck-iipi are you still owrking on this |
|
/update |
|
closing this as no response |
Description
Related issue(s)
Fixes #3158
Summary by CodeRabbit
New Features
ResponsiveImagecomponent for improved image handling across sponsor sections.<img>tags with theResponsiveImagecomponent in Gold, Silver, and general Sponsors sections.Bug Fixes
data-testidattributes todataTestIdfor consistency and improved testing capabilities.