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

Fetching and rendering of AreWeHeadlessYetTopicPage #5

Merged
merged 19 commits into from Apr 22, 2022
Merged

Conversation

Tijani-Dia
Copy link
Collaborator

@Tijani-Dia Tijani-Dia commented Feb 22, 2022

There is a topics section in the homepage where is listed all the topics relevant to headless builds with Wagtail. We also have a dedicated topic page which provides more details about each topic.
The goal of this PR is to fetch the topic pages from the backend in wagtail.org and render them (Ticket 19 on CodeBase) i.e build the dedicated topic page.

The work done here is based on what have been done in the sections branch hence the target. That also explains why the CI fails; fixing it in that branch will get it resolved here too.

@thibaudcolas
Copy link
Member

@Tijani-Dia I don’t think this is reviewable at this stage – as I said in #1, please make sure to always describe the changes you’re making in the pull request description. Here I’d expect you to say:

  • What the PR is trying to achieve overall
  • Which tickets it’s for
  • How you’ve tested it
  • Why it’s pointing at the sections branch (what is it for?) rather than main? What’s the plan to merge what is inside section?
  • Why the CI builds are failing / should we ignore this or raise it with you / what’s your plan to get them passing.

@Tijani-Dia
Copy link
Collaborator Author

@thibaudcolas I've added a description to this PR. Let me know if you want more details.

I've tested it locally and in the preview environment.

Copy link

@patrickcuagan patrickcuagan left a comment

Choose a reason for hiding this comment

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

Thanks Tidiane! I've left a few comments and questions.

lib/topics.ts Outdated Show resolved Hide resolved
lib/topics.ts Outdated Show resolved Hide resolved
Comment on lines 30 to 36
if (blockType === 'text') {
return (
<div key={i} className={styles.text}>
<RichTextBlock
key={i}
{...(block as RichTextBlockItem)}
/>
</div>
);
}

Choose a reason for hiding this comment

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

Question

Why can't we add this to the BLOCKS object above?

Copy link
Collaborator Author

@Tijani-Dia Tijani-Dia Mar 3, 2022

Choose a reason for hiding this comment

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

The styling for the sections and plain text/paragraphs is different and we don't pass block.value to the RichTextBlock component currently, we're only passing the block itself.

Choose a reason for hiding this comment

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

Right my bad - I didn't notice that the className changed.

Suggestion

What do you think about adding <div className={styles.text}> inside the RichTextBlock component.

If I understand it correctly, with how it's built now, every time you want to render a RichTextBlock, you'll also need to declare the div that encloses it. (The same applies for all the blocks in BLOCKS and TopicsBlock)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I preferred styling all those blocks in Streamfield.module.scss because they share a lot of styles rather than doing it separately.

components/StreamField/types.ts Show resolved Hide resolved
lib/fetch.ts Outdated Show resolved Hide resolved
pages/[slug].tsx Outdated Show resolved Hide resolved
pages/[slug].tsx Show resolved Hide resolved
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.

None yet

3 participants