fix: remove Twitter link for ambassador without a Twitter account #3859
Conversation
Fixes #3828 Remove the Twitter link for the ambassador without a Twitter account. * Update `config/AMBASSADORS_MEMBERS.json` to remove the `twitter` field for the ambassador without a Twitter account. * Modify `pages/community/ambassadors/index.tsx` to add a conditional check to only generate a Twitter URL if the ambassador has a Twitter account. * Update `pages/community/ambassadors/index.tsx` to add a conditional check to only display the Twitter link if the profile has a Twitter account.
Recxsmacx
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve and
derberg
as code owners
WalkthroughThis update adds a Twitter handle entry for Manuel Ottlik in the ambassadors configuration JSON file. Additionally, it modifies the rendering logic in the community ambassadors page to always display the Twitter link, regardless of the validity of the URL. Furthermore, it includes corrections in the AsyncAPI document generation functions and updates to the formatting of hook descriptions in the documentation. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 180000ms (4)
🪧 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
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3859 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 667 667
Branches 113 113
=========================================
Hits 667 667 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pages/community/ambassadors/index.tsx (1)
32-32: Simplify the Twitter URL assignmentThere's a redundant conditional check in this line. Since this code is already inside an
if (userData.twitter)block, we knowuserData.twitteris truthy at this point, making the additional check unnecessary.- userData.twitterUrl = userData.twitter ? `https://www.twitter.com/${userData.twitter}` : null; + userData.twitterUrl = `https://www.twitter.com/${userData.twitter}`;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/AMBASSADORS_MEMBERS.json(0 hunks)pages/community/ambassadors/index.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- config/AMBASSADORS_MEMBERS.json
⏰ Context from checks skipped due to timeout of 180000ms (1)
- GitHub Check: Lighthouse CI
🔇 Additional comments (1)
pages/community/ambassadors/index.tsx (1)
144-154: Great implementation of conditional Twitter link renderingThe conditional rendering of the Twitter link ensures that ambassadors without a Twitter account don't have an empty or broken link displayed, which improves the user experience.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
markdown/docs/tools/generator/hooks.md (1)
14-16: Update Relative URLs and Link Descriptions.
The table rows for the hooksgenerate:before,generate:after, andsetFileTemplateNamehave been updated to use relative URLs (e.g.,(api)instead of an absolute link) and a refined description forsetFileTemplateName. This change improves consistency and clarity in the documentation. Please verify that the relative links resolve as intended and that the description now accurately referencesfile-templates(omitting the.mdextension) to align with the updated documentation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
config/AMBASSADORS_MEMBERS.json(0 hunks)config/AMBASSADORS_MEMBERS.json(1 hunks)markdown/docs/tools/generator/asyncapi-document.md(1 hunks)markdown/docs/tools/generator/hooks.md(1 hunks)markdown/docs/tools/generator/migration-nunjucks-react.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- markdown/docs/tools/generator/migration-nunjucks-react.md
🚧 Files skipped from review as they are similar to previous changes (2)
- config/AMBASSADORS_MEMBERS.json
- config/AMBASSADORS_MEMBERS.json
⏰ Context from checks skipped due to timeout of 180000ms (1)
- GitHub Check: Lighthouse CI
🔇 Additional comments (2)
markdown/docs/tools/generator/asyncapi-document.md (2)
71-71: Corrected Path ResolutionThe updated
path.resolvecall correctly removes the extraneous single quote. The use ofpath.resolve('./', outputFileName)ensures that the file path is properly constructed without any syntax errors.
74-74: Proper Function ClosureThe addition of the closing brace
}properly terminates thecreateAsyncapiFilefunction. This fix resolves the syntax issue that previously could have led to runtime errors.
|
I was able to find their id> |
TRohit20
left a comment
TRohit20
left a comment
There was a problem hiding this comment.
But @sambhavgupta0705 Do we really need this PR to be merged? It could be that Manuel had not given his twitter ID on purpose or for a reason.
Plus, I clearly remember, we have one PR currently on the verge of being merged which essentially displays only the available info on the card without blank links to click on. For instance, in this case, if Manuel didn't provide a twitter/X ID, on his display card on the website, 'Twitter' Icon/Link will not be displayed. So I think this PR might be redundant. WDYT?
|
@sambhavgupta0705 it's just an update if manuel didn't provided his x account we can remove it too? Or we can add another social link of everyone like mail-id? |
|
@sambhavgupta0705 @TRohit20 or it - might be possible that he's not active on X either we can remove the X button or add value in it either we can replace it with his mail id |
Yes that can be a reason ,and I feel we should remove the Twitter link ,if he raise a concern about it then we can create another PR for this
Yes ,so should we keep this on hold |
|
@TRohit20 I think we should not do anything we social links and let the community managers look after this IMO we should close all issues like this |
|
Totally agreed. That is the reason I raised this concern now. This is not entirely to our discretion to decide and merge tbh. Due to which, I am actually closing this PR and the reference issue. |
4 participants
Fixes #3828
Able to find the social handles of the ambassadors and updated their values so that they can redirect them to the correct locations