Website: New page to present tech stack #976
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughA new tech stack page has been added to the Social Income website, featuring a comprehensive overview of the technologies and tools used by the organization. The implementation includes a hero section, a tabbed tech list interface, and a JSON configuration file that defines the tech stack details. The page allows users to view all technologies and filter specifically for donated technologies. Changes
Sequence DiagramsequenceDiagram
participant User
participant TechStackPage
participant Hero
participant TechList
participant TechCard
User->>TechStackPage: Access Tech Stack Page
TechStackPage->>Hero: Render Hero Section
TechStackPage->>TechList: Render Tech List
TechList->>TechCard: Create Tech Cards
User->>TechList: Switch Tabs
TechList->>TechCard: Filter Tech Cards
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 (
|
|
Visit the preview URL for this PR (updated for commit 3db2d98): https://si-admin-staging--pr976-lvonlanthen-techstac-xbos5djl.web.app (expires Thu, 09 Jan 2025 11:46:38 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
lvonlanthen
force-pushed
the
lvonlanthen/techstack-page
branch
from
448dd78 to
a5b0e02
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
website/src/app/[lang]/[region]/(website)/techstack/(sections)/techlist.tsx (1)
9-16: Enhance type safety for tech entry interface.The current type definition could be more strict to prevent potential runtime errors.
Consider this enhanced type definition:
type TechEntryJSON = { - title: string; - description: string; - link: string; - logo: string; - donated: boolean; + readonly title: string; + readonly description: string; + readonly link: string; + readonly logo: string; + readonly donated: boolean; };website/src/app/[lang]/[region]/(website)/techstack/(sections)/techcard.tsx (2)
5-16: Consider adding type constraints for URLs and image files.The type definitions could be more specific to prevent invalid inputs.
type TechCardProps = { title: string; description: string; - link: string; - logo: string; + link: `https://${string}` | `http://${string}`; + logo: `${string}.${'png' | 'jpg' | 'svg' | 'webp'}`; donated: boolean; translations: TechCardTranslations; };
39-39: Simplify the className conditional.The double negation is unnecessary and can be simplified.
-<div className={'' + (!!logo ? 'w-fit basis-3/4' : '')}> +<div className={logo ? 'w-fit basis-3/4' : ''}>🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
website/public/assets/tech/google-nonprofit.pngis excluded by!**/*.png
📒 Files selected for processing (5)
shared/locales/en/website-techstack.json(1 hunks)website/src/app/[lang]/[region]/(website)/techstack/(sections)/hero.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/techstack/(sections)/techcard.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/techstack/(sections)/techlist.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/techstack/page.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- shared/locales/en/website-techstack.json
🧰 Additional context used
🪛 Biome (1.9.4)
website/src/app/[lang]/[region]/(website)/techstack/(sections)/techcard.tsx
[error] 39-39: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (1)
website/src/app/[lang]/[region]/(website)/techstack/page.tsx (1)
5-12: LGTM! Clean and well-structured page component.
The implementation follows Next.js app router conventions and properly handles internationalization parameters.
website/src/app/[lang]/[region]/(website)/techstack/(sections)/techlist.tsx
Outdated
Show resolved
Hide resolved
website/src/app/[lang]/[region]/(website)/techstack/(sections)/techcard.tsx
Outdated
Show resolved
Hide resolved
website/src/app/[lang]/[region]/(website)/techstack/(sections)/techcard.tsx
Show resolved
Hide resolved
website/src/app/[lang]/[region]/(website)/techstack/(sections)/techcard.tsx
Show resolved
Hide resolved
|
@DarkMenacer would you have time to give this PR a first review? @lvonlanthen did his first PR for this repo and the entire issue is very similar to the partners page (cards). Let us know and we plan accordingly. |
|
Sure @ssandino, on it right away |
|
@lvonlanthen, I think these 3 should be enough for now |
During initial render or while translation is loading, the `translator` or `techArray` variables might still be null. In this case, display a loading spinner that will be replaced with the actual content once it is available. Based on #976 (comment)
lvonlanthen
force-pushed
the
lvonlanthen/techstack-page
branch
from
1cab6a4 to
3ef9ff5
Compare
@DarkMenacer I fixed the two unresolved conversations, but I am not sure which 3 issues you mean. Also, I guess I would need some pointers on other things to improve. Thanks! |
website/src/app/[lang]/[region]/(website)/techstack/(sections)/techlist.tsx
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,50 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
Is there any particular reason you are making this a client-side component?
I think we can instead make techcard.tsx into a client-side component and store it in a separate folder (components) instead of (sections)
There was a problem hiding this comment.
I encountered issues with client/server components and async/await when adding the tabs. I was able to fix it by adding 'use client' (and replacing Translator.getInstance with useTranslator), but I don't know React well enough to tell if this is the correct fix. I saw that 'use client' is used quite often throughout the code and it seems to do the trick.
When turning techcard.tsx into a client-side component, I think (again, limited React knowledge on my side) I would also need to do the same with techlist.tsx because I am using useState to keep track of the tab state to filter the list and useState only works for client components, correct?
Should I still try to refactor both files into components? And to improve my understanding of React, what are the reasons for not using 'use client'?
There was a problem hiding this comment.
Hi @lvonlanthen, I completely relate with you. Even I had some difficulty in understanding the difference between client-side and server-side components. Michael had shared a link with me which really helped me clarify my doubts. I shall do the same: NextJS Components.
The main idea here is: Instead of sending the entire component over from the server, just send the data and load the component using browser's resources (i.e. client-side). It is just a good way to reduce server load
That is why I believe that techcard can be a client-side component and techlist can do the data fetching part.
Let me also share one of the pages I worked on recently for you to understand the implementation there.
Here's the server-side component: Section-1 Cards List
and here's the client-side component: Section Card
You can also go through other sections (2 and 3) and the web page so that the point becomes crystal clear.
Finally, feel free to ping me on Slack as well if you would like more discussion around this. Always happy to help 😊.
Oh sorry, it seems I hadn't submitted my review before, my bad |
It was necessary to replace `Translator.getInstance` with `useTranslator` and add `'use client'`, otherwise there were issues with client/server components and async/await. Not sure if this is the correct way to solve this, but it seems to work.
During initial render or while translation is loading, the `translator` or `techArray` variables might still be null. In this case, display a loading spinner that will be replaced with the actual content once it is available. Based on #976 (comment)
The property `region` is not used by the child components and does not need to be passed along.
lvonlanthen
force-pushed
the
lvonlanthen/techstack-page
branch
from
bf5dc87 to
3db2d98
Compare
Summary by CodeRabbit
New Features
Herocomponent to showcase titles and subtitles related to the tech stack.TechCardcomponent to display individual technology details.TechListcomponent with a tabbed interface to filter technologies.HeroandTechListcomponents.Bug Fixes