Break up LandingPage components into sub-components #214
Conversation
There was a problem hiding this comment.
Nice work!
I left some comments, most of which can also be applied to the rest of the code.
Some general things to do:
- put files under
landing-page/ - fix formatting (take a look at how it was before)
- don't use
any, define TS types/interfaces instead - drop unneccesary
<> - let's extract svgs, I know it is a bit beyond initial scope of this PR but it makes sense in order to improve readibility.
I think this should be it!
|
@Martinsos I made all the changes you mentioned and also seperates svgs in icons folder |
|
@Martinsos I made all the changes but having issue in indentation, in my IDE code is in good format, but while pushing here code is in bad format :/ i have no idea how can fix it... |
Probably has to do with spaces / tabs! What I would recommend in general is using spaces and have editor show them exactly as they are, to avoid issues like this. |
There was a problem hiding this comment.
Ok @manjhss nice job! I would say this is all, except for following things:
- Indentation is off.
- We have conflicts with
mainbranch. - We need to update the demo app (opensaas-sh/app) and also docs if needed.
Regarding (1), if you don't manage to fix it (and you said you were having trouble with that), no big deal, we can fix indentation ourselves before the merge.
Regarding (2), since the big vertical reorganization is still happening, might be best if we wait just a bit for it to be finished and then resolve the conflicts. New version of open-saas should be released in a couple of days, so we can do it after that.
Regarding (3) -> we will take care of that as it is a bit complex!
So this is it really -> thanks for the good work, let's give it a couple of days of rest till new version of OpenSaas is released, and then we will take over the PR and do final stuff that is needed to merge this!
|
Ok this should be it! I fixed indentation (replaced tabs with spaces), updated the demo app (app_diff/), and there were no updates for the docs needed I believe. |
|
|
||
| <div className='mx-auto grid max-w-lg grid-cols-2 items-center gap-x-8 gap-y-12 sm:max-w-xl md:grid-cols-4 sm:gap-x-10 sm:gap-y-14 lg:mx-0 lg:max-w-none'> | ||
| { | ||
| [<SalesforceLogo />, <PrismaLogo />, <AstroLogo />, <OpenAILogo />].map((logo, index) => ( |
| @@ -0,0 +1,37 @@ | |||
| interface Feature { | |||
There was a problem hiding this comment.
Looking very clean and organized. Thanks so much for your hard work @manjhss 🙏
@Martinsos the update to the app_diff is just a dead link I fixed. Ran the tests and they all passed. Everything looks good :)
Description
This PR fixes Break up LandingPage.tsx components into multiple sub-components issue.
Fixes: #147
Contributor Checklist