Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

RalitsaIlieva
Copy link
Contributor

@RalitsaIlieva RalitsaIlieva commented Feb 12, 2024

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:

Before After
Paste screenshot Paste screenshot

Testing

Steps to test

Affected urls

Environment

New environment variables:

  • NEW_ENV_VAR: env var details

New or updated dependencies:

Dependency name Previous version Updated version Details
dependency/name v1.0.0 v2.0.0

Copy link

github-actions bot commented Feb 12, 2024

❌ Not all tests have run for this PR. Please add the run tests label to trigger them.

@igoychev igoychev added the run tests Allows running the tests workflows for forked repos label Mar 4, 2024
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Mar 4, 2024
Copy link
Member

@sashko9807 sashko9807 left a 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.

@@ -89,6 +89,21 @@
"download": "Изтеглете",
"allow-donation-on-complete": "Разрешете дарения след достигане на сумата"
},
"subscribe": {
Copy link
Member

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.

Copy link
Contributor Author

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)
Copy link
Member

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)
Copy link
Member

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 && (
Copy link
Member

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'}}}

Copy link
Contributor Author

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')}
Copy link
Member

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')

Copy link
Contributor Author

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>
Copy link
Member

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

Copy link
Contributor Author

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>
Copy link
Member

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.

Copy link
Contributor Author

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}>
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@@ -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'])),
Copy link
Member

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?

Copy link
Contributor Author

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

'news',
'campaigns',
'campaign-types',
'auth',
Copy link
Member

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?

Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

News redesign
3 participants