-
Notifications
You must be signed in to change notification settings - Fork 3
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/landing page build #18
Conversation
IgboPharaoh
commented
Sep 9, 2024
- Enhance algorithms and landing page corrections
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@Emmanuel-Develops please review |
@@ -1,3 +1,5 @@ | |||
"use server"; |
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.
Components are server components by default. Hence they do not require the "use server" directive.
However, implementing this change locally exposed some issues, after going down a rabbit hole of debugging, the problem stemmed from barrel exporting server and client components from a single entry point src/app/components/index.tsx
.
Without marking as "use server" we get the long page download (18mb).
A fix would be segregating and grouping the imports, ideally these components should be under a sub-directory for incremental building. i.e a components/layout
containing header
and footer
components and a couple other sub-directories.
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.
Actually, I added the "use server" directive because it prevented the long page download of (18mb) which was the major issue I wanted to fix, but I'll like to know the other issues this change exposed locally to be better informed.
To be clear, you're asking for a change to the export pattern of the components we're currently using to something like having different files components are exported from.
Example :
- components/server
- components/client
- components/layout
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.
Another issue noticed by this export is in ExploreTranscripts
there's also a "use server" for ExploreTranscripts
and removing that throws an error saying fs module not found
which isn't right as it should be a server component being imported into src/app/page.tsx
a server-rendered page.
Yes, a change in export pattern.
components/server
, component/client
won't be the right abstraction as some files are colocated and more reasonable if they are somewhat in the same directory.
E.g ExploreTranscripts
and ExploreTranscriptsClient
.
Example:
- components/layout/header
- components/layout/footer
- components/layout/wrapper
- components/landing/...allTranscripts
This should come in after add-hugo
PR
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.
Alright, I refactor the component imports not to use a single entry point and group colocated files accordingly after the add-hugo
PR
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.
@IgboPharaoh kindly add the refactoring in this PR
cc: @0tuedon get ready for a rebase 😌. #15 is in staging so these refactors will be added to this PR and then merged to staging.
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.
poor me 🥲🥲
LGTM |
aceff6c
to
43752b7
Compare
* add required assets * enhance contentlayer algorithm * add design corrections * filter languages that are not english * remove unnecessary console.log * capitalize titles and sort types * refactor code structure * add product favicon
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.
This looks great! All the issues that I listed in my latest review have been (almost) solved 🚀
There is this issue that still remains
Sub-Section Body Text: The text stretches too much on larger screens. Refer to the Bitcoin Search landing page for a similar layout; we should aim for a consistent approach.
for which I didn't do a good job explaining. We can hop on a quick call at some point so that I show you what I mean.
<section className='w-[40%] flex flex-col gap-10 max-md:w-full max-md:gap-6 max-md:px-4'> | ||
<section className='text-black flex flex-col gap-6 max-md:max-w-[368px]'> | ||
<h1 className='2xl:text-[72px] text-[64px] leading-[130%] max-xl:text-5xl max-xl:leading-[130%] max-[953px]:text-[38px] max-[953px]:leading-[130%] max-md:text-[42px]'> | ||
Unlock the treasure trove of technical bitcoin transcripts | ||
</h1> | ||
<p className='text-xl max-lg:text-lg max-md:text-base'> | ||
<span className='font-bold'>1042+ transcripts</span> growing every day. Thanks to people <br /> like you. | ||
<span className='font-bold'>1042+ transcripts</span> growing every day. <br /> Thanks to people like you. |
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.
Now the second sentence is always at a separate line. I think that we can use white-space: nowrap;
for the second sentence so that the entire sentence or block of text stays together and doesn't break between lines.
Another option is to use non-breaking spaces (
) between the words of the second sentence.
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.
I don't think the second sentence breaks on any screen size, but we can add a text-nowrap to the entire sentence to ensure the block stays together.
The text only breaks from 803px - 768px.
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.
I think we should move all conversations to the production pr, so these can be tracked
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.
I don't think the second sentence breaks on any screen size, but we can add a text-nowrap to the entire sentence to ensure the block stays together. The text only breaks from 803px - 768px.
You have the <br />
there so the second sentence is always on a new line.
* simplify contentlayer.config (#6) a simplification of the config alongside some changes to the pages in order to show the structure based on the new simplified implementation. * Feat/style: landing page (#7) * build: install bdp-ui icons * build: set config requirements * style: add assets and icons * chore: remove redundant styles, add menu data * chore: add landing page components * chore: create landing page with responsive styles * fix: build error * replace image assets * add mobile menu and app menu * install bdp-ui from npm * complete landing page requirements * remove search icon, change image path and add priority prop * feat: Add hugo built pages (#15) * feat: fetch and render hugo built pages in nextJs * refactor: use rewrites in place of [...slug] * Fix/landing page build (#18) * add required assets * enhance contentlayer algorithm * add design corrections * filter languages that are not english * remove unnecessary console.log * capitalize titles and sort types * refactor code structure * add product favicon * chore: rm commented code * fix: hash navigation, change stock image to btc prague conference image (#24) --------- Co-authored-by: Andreas <[email protected]> Co-authored-by: Solomon eze <[email protected]>