fix: fixed Newsletter page labels #3238
Conversation
hkv24
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3238--asyncapi-website.netlify.app/ |
asyncapi-bot
requested review from
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
|
/update |
WalkthroughThe changes made in the Changes
Assessment against linked issues
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 (1)
components/NewsletterSubscribe.tsx (1)
137-150: Document the decision to use static strings instead of translation keysWhile the changes successfully fix the labels on the Newsletter page as per the PR objectives, they introduce a potential inconsistency in the internationalization approach of the component.
Consider the following points:
- Document the reasoning behind using static strings for these specific elements while keeping translations for others.
- Ensure that this approach aligns with the long-term internationalization strategy of the project.
- If the intention is to remove translations for the entire component, consider updating the remaining translated elements for consistency.
This documentation will help maintain clarity about the internationalization approach and prevent confusion in future development.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
.github/workflows/scripts/mailchimp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
- components/NewsletterSubscribe.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
components/NewsletterSubscribe.tsx (2)
144-144:⚠️ Potential issueRemove unnecessary translation function for static text
Similar to the previous comment, the
t()function is unnecessary for the static email placeholder text.Apply the same change as suggested for the name input placeholder:
-placeholder={t('Your Email')} +placeholder={'Your Email'}
150-150:⚠️ Potential issueRemove unnecessary translation function for static text
As with the previous two comments, the
t()function is unnecessary for the static button text.Apply the same change as suggested for the input placeholders:
-text={t('Subscribe')} +text={'Subscribe'}
| @@ -134,20 +134,20 @@ export default function NewsletterSubscribe({ | |||
| <InputBox | |||
| inputType={InputTypes.TEXT} | |||
| inputName='name' | |||
| placeholder={t('newsletterCTA.nameInput')} | |||
| placeholder={t('Your Name')} | |||
There was a problem hiding this comment.
Remove unnecessary translation function for static text
The placeholder text has been changed from a translation key to a static string, which aligns with the PR objective. However, wrapping a static string with t() is unnecessary and defeats the purpose of internationalization.
Consider removing the t() function:
-placeholder={t('Your Name')}
+placeholder={'Your Name'}This change will maintain the intended functionality while simplifying the code.
📝 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.
| placeholder={t('Your Name')} | |
| placeholder={'Your Name'} |
|
@coderabbitai please create a new issue for this |
|
closing this as no response from contributor |
|
Tip For best results, initiate chat on the files or code changes. @sambhavgupta0705, I've created a new issue to track the performance improvements based on the Lighthouse report: #3356 🔗 Analysis chain@sambhavgupta0705, I will create a new issue for this. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
gh issue create --title "Improve Performance Based on Lighthouse Report" \
--body "As observed in the [Lighthouse report](https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1728449109632-14921.report.html) from PR #3238 (https://github.com/asyncapi/website/pull/3238), the Performance score is low (42). This issue is to track efforts to improve the website's performance. Original request by @sambhavgupta0705." \
--assignee sambhavgupta0705
Length of output: 517 |
Description
Updated the placeholder values of the two input fields and the button on '/newsletter' route.
Screenshot
Fixes #3236
Summary by CodeRabbit