-
Notifications
You must be signed in to change notification settings - Fork 92
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
news-redesign #1718
base: master
Are you sure you want to change the base?
news-redesign #1718
Conversation
❌ Not all tests have run for this PR. Please add the |
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.
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
@@ -89,6 +89,21 @@ | |||
"download": "Изтеглете", | |||
"allow-donation-on-complete": "Разрешете дарения след достигане на сумата" | |||
}, | |||
"subscribe": { |
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.
Subscription related translations, are already added in the common namespace.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'}}}
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.
ok
</Grid> | ||
<Grid> | ||
<SubscribeButton onClick={() => setSubscribeOpen(true)} variant="contained"> | ||
{t('campaigns:cta.subscribe-general-newsletter-button')} |
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.
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')
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.
Ok
}> | ||
{article.title} | ||
</Typography> | ||
<QuillStypeWrapper> |
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.
Following the new the article text doesn't seem to be needed
Reference news design
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.
Yes. Removed
</Grid> | ||
<Grid container item> | ||
<Grid container item md={7} xs={12}> | ||
<ArticleSection> |
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.
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.
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.
ok
{/* } */} | ||
</Grid> | ||
<Grid item md={9} xs={7} container> | ||
<ArticleSection id={article.id}> |
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.
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.
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.
OK
src/pages/page/[slug].tsx
Outdated
@@ -17,7 +17,7 @@ export const getServerSideProps: GetServerSideProps = async ({ params, locale }) | |||
return { | |||
props: { | |||
page, | |||
...(await serverSideTranslations(locale ?? 'bg', ['common', 'blog'])), | |||
...(await serverSideTranslations(locale ?? 'bg', ['common', 'blog', 'campaigns'])), |
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 is campaigns namespace required?
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 remember. Removed
src/pages/campaigns/news/index.tsx
Outdated
'news', | ||
'campaigns', | ||
'campaign-types', | ||
'auth', |
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 is auth namespace required?
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.
Removed it. It seems it is not needed
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/name
v1.0.0
v2.0.0