Conversation
|
❌ Not all tests have run for this PR. Please add the |
github-actions
bot
removed
the
run tests
sashko9807
left a comment
sashko9807
left a comment
There was a problem hiding this comment.
Hello and sorry for the delayed review.
I made some remarks and questions .code-wise, would you please look at them.
Regarding the overall layout, there seems to be inconsistence, with what you've done so far, and the design given by the design team link, so please look at what needs to be done layout-wise as well.
If you have any questions regarding the requested changes or need some help, feel free to contact me.
public/locales/bg/campaigns.json
Outdated
| "download": "Изтеглете", | ||
| "allow-donation-on-complete": "Разрешете дарения след достигане на сумата" | ||
| }, | ||
| "subscribe": { |
There was a problem hiding this comment.
Subscription related translations, are already added in the common namespace.
There was a problem hiding this comment.
Ok. I removed it from the campaigns.json
| data: campaign, | ||
| isLoading: isLoadingCampaignData, | ||
| isError: isErrorCampaignData, | ||
| } = useViewCampaignById(article.campaign.id) |
There was a problem hiding this comment.
Not needed. If you want to take the campaign image, you can include it in the article response, by adding necessary campaignFiles relation. This would look something like this:
this.prisma.campaignNews.findMany({
where: { state: CampaignNewsState.published },
orderBy: { publishedAt: 'desc' },
take: this.RECORDS_PER_PAGE,
skip: Number((currentPage - 1) * this.RECORDS_PER_PAGE),
select: {
..................
campaign: {
..............
campaignFiles: {
where: { role: CampaignFileRole.campaignListPhoto },
}
}
}
})Side note:
Keep in mind that calling an endpoint within a loop is not considered a good practice in general, as it adds unnecessary stress to the backend server(n+1 problem).
| data: campaign, | ||
| isLoading: isLoadingCampaignData, | ||
| isError: isErrorCampaignData, | ||
| } = useViewCampaignById(article.campaignId) |
There was a problem hiding this comment.
This is protected route.
You can include the campaign slug, in the article response, and use useViewCampaign(article.campaign.slug), to fetch the necessary data.
| </Grid> | ||
| </Grid> | ||
| {article.newsFiles.length > 0 && ( | ||
| {isDesktop && ( |
There was a problem hiding this comment.
Please opt-in for a single responsive layout, rather than having a mobile,desktop layouts. In case some element should be visible only for mobile, or only for desktop, you can add breakpoints just for that specific element, by doing.
In the example below a breakpoint is added to that element and for viewports < 600px, the display is none, and for viewports >=600px the display is flex.
<Grid item sx={{display:{xs:'none', sm:'flex'}}}| </Grid> | ||
| <Grid> | ||
| <SubscribeButton onClick={() => setSubscribeOpen(true)} variant="contained"> | ||
| {t('campaigns:cta.subscribe-general-newsletter-button')} |
There was a problem hiding this comment.
Subscription related translations, have been moved to common namespace
Change
t('campaigns:cta.subscribe-general-newsletter-button')to
t('common:notifications.subscribe-general-newsletter-button')| }> | ||
| {article.title} | ||
| </Typography> | ||
| <QuillStypeWrapper> |
There was a problem hiding this comment.
Following the new the article text doesn't seem to be needed
Reference news design
| </Grid> | ||
| <Grid container item> | ||
| <Grid container item md={7} xs={12}> | ||
| <ArticleSection> |
There was a problem hiding this comment.
ArticleSection should be the parent container, as it gives the necessary styling of used classes, thus avoiding the inline styling, and making the code more readable.
| {/* } */} | ||
| </Grid> | ||
| <Grid item md={9} xs={7} container> | ||
| <ArticleSection id={article.id}> |
There was a problem hiding this comment.
ArticleSection should be the parent container, as it gives the necessary styling of used classes, thus avoiding the inline styling, and making the code more readable.
src/pages/page/[slug].tsx
Outdated
| props: { | ||
| page, | ||
| ...(await serverSideTranslations(locale ?? 'bg', ['common', 'blog'])), | ||
| ...(await serverSideTranslations(locale ?? 'bg', ['common', 'blog', 'campaigns'])), |
There was a problem hiding this comment.
Why is campaigns namespace required?
There was a problem hiding this comment.
I don`t remember. Removed
src/pages/campaigns/news/index.tsx
Outdated
| 'news', | ||
| 'campaigns', | ||
| 'campaign-types', | ||
| 'auth', |
There was a problem hiding this comment.
Why is auth namespace required?
There was a problem hiding this comment.
Removed it. It seems it is not needed
3 participants
Closes #1671
(First, an api change that I made should be approved and merged)
Motivation and context
We want better design for the news section
Screenshots:
Testing
Steps to test
Affected urls
Environment
New environment variables:
NEW_ENV_VAR: env var detailsNew or updated dependencies:
dependency/namev1.0.0v2.0.0