-
Notifications
You must be signed in to change notification settings - Fork 14
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
Projects redesign/project details #2183
base: projects-redesign/list-screen
Are you sure you want to change the base?
Projects redesign/project details #2183
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
Check again for design consistency. |
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.
Looks good to merge. Once in complete context we will need to review fontsizes, some seem quite small.
@mariahosfeld I think we need to list down information that should be seen in the project details for conservation and tree projects. We have 2 main sections below the image slider (KeyInfo and AdditionalInfo), and currently KeyInfo is shown only for tree projects. That logic isn't correct, but I think we need a definite list of information that can be seen. |
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.
@sunilsabatp Please go through my feedback.
src/features/projectsV2/ProjectSnippet/styles/ProjectSnippet.module.scss
Outdated
Show resolved
Hide resolved
src/features/projectsV2/ProjectDetails/components/microComponents/ProjectSpendingItem.tsx
Outdated
Show resolved
Hide resolved
src/features/projectsV2/ProjectDetails/components/microComponents/ExternalCertificationItem.tsx
Outdated
Show resolved
Hide resolved
src/features/projectsV2/ProjectDetails/components/ProjectDownloads.tsx
Outdated
Show resolved
Hide resolved
src/features/projectsV2/ProjectDetails/components/microComponents/SingleContactDetail.tsx
Outdated
Show resolved
Hide resolved
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.
@sunilsabatp Please go through my feedback.
Please make sure that all datapoint available via the API response are shown in the project details. For a conservation project several are missing. |
.projectInfoSection { | ||
padding: 15px; | ||
background: rgba(255, 255, 255, 1); | ||
border-radius: 16px; | ||
margin-top: 20px; | ||
display: flex; | ||
flex-direction: column; | ||
grid-area: 15px; | ||
gap: 15px; | ||
box-shadow: 0px 0px 10px 0px rgba(0, 0, 0, 0.1); | ||
@include xsPhoneView { | ||
box-shadow: none; | ||
margin-left: 0px; | ||
padding: 15px 30px; | ||
} | ||
} |
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.
Move the projectInfoSection styles to ProjectInfo.module.scss - that makes more sense in terms of heirarchy, as ProjectInfoSection lies within the /components folder which uses styles from /styles.
/> | ||
</div> | ||
) : ( | ||
<Skeleton className={styles.projectInfoSkeleton} /> |
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.
Since we have a ProjectInfoSection component, the projectInfoSkeleton
class could be renamed to projectDetailsSkeleton
as that is what it appears as a placeholder for
.projectListControlsMobile { | ||
height: 26px; | ||
gap: 10px; | ||
} |
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.
Why are we giving a height of 26px here, when the contents within are 28px?
.marginTop10 { | ||
margin-top: 10px; | ||
} | ||
|
||
.marginTop20 { | ||
margin-top: 20px; | ||
} |
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.
- Still not sure why we need 2 different margins.
- Also, while using utility classes is fine, using a mixture of utility classes and other classes can cause confusion. In this scenario, I feel it would be better to use a specific class for impersonation mode.
const financialAndCertificationData = [ | ||
{ |
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.
Let's rename financialAndCertificationData
--> downloadsRenderConfig
.
It does not represent simple data, but a configuration of what should render and when, and we can also add another item (e.g. progressReports) without having to rename everything to make sense.
const [singleProject, setSingleProject] = useState<ProjectExtend | null>( | ||
null | ||
); |
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.
Update the ProjectExtend type to ExtendedProject here as well.
const handleBackButton = useCallback((e: MouseEvent) => { | ||
e.stopPropagation(); | ||
if (window.history.length > 0) { | ||
window.history.back(); | ||
} else { | ||
window.location.href = '/'; | ||
} | ||
}, []); |
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.
The back button logic does not seem right, perhaps this can be taken up in a separate PR though.
return Boolean( | ||
(isTreeProject && metadata.yearAbandoned) || | ||
(isTreeProject && metadata.firstTreePlanted) || | ||
(isTreeProject && metadata.plantingDensity) || | ||
(isTreeProject && metadata.maxPlantingDensity) || | ||
(isTreeProject && | ||
metadata.plantingSeasons && | ||
metadata.plantingSeasons.length > 0) || | ||
(isConservationProject && | ||
metadata.activitySeasons && | ||
metadata.activitySeasons.length > 0) | ||
); |
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.
Can this be more readable, with less duplication?
const shouldRenderAdditionalInfo = useMemo(() => { | ||
const { | ||
mainChallenge, | ||
siteOwnerName, | ||
longTermPlan, | ||
acquisitionYear, | ||
motivation, | ||
mainInterventions, | ||
} = metadata; | ||
return Boolean( | ||
mainChallenge || | ||
siteOwnerName || | ||
(isTreeProject && | ||
metadata.siteOwnerType && | ||
metadata.siteOwnerType?.length > 0) || | ||
(isConservationProject && | ||
metadata.landOwnershipType && | ||
metadata.landOwnershipType?.length > 0) || | ||
(isTreeProject && metadata.degradationCause) || | ||
longTermPlan || | ||
acquisitionYear || | ||
motivation || | ||
(mainInterventions && mainInterventions?.length > 0) | ||
); | ||
}, [metadata]); |
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.
Can this be more readable?
} = project; | ||
const isTreeProject = purpose === 'trees'; | ||
const isConservationProject = purpose === 'conservation'; | ||
const isRestorationProject = purpose === 'trees' && unitType === 'm2'; |
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.
The isRestorationProject
condition does not depend on unitType
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.
@sunilsabatp Please go through all open comments, including the ones from earlier
This PR takes care the Project info section which includes
how to test ?
visit ->
/locale/prd/[project slug]