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

feature: add grouping block #257

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

MarleenEliza
Copy link

@MarleenEliza MarleenEliza commented Mar 13, 2025

Changes

This is meant to replace a part of the Page Partial Block functions, but since the editing of the Page Partial Block is a breaking change that will impact already existing Page Partial Blocks, this change will be dealt with in a different PR.
This PR only adds a GroupingBlock which is a non-breaking change.

Associated issue

#256 (non breaking half)

How to test

Checklist

  • I have performed a self-review of my own code
  • I have made sure that my PR is easy to review (not too big, includes comments)
  • I have made updated relevant documentation files (in project README, docs/, etc)
  • I have added a decision log entry if the change affects the architecture or changes a significant technology
  • I have notified a reviewer

@MarleenEliza MarleenEliza force-pushed the 256-add-groupingblock branch from 7d88d5e to 77771c2 Compare March 13, 2025 09:03
Copy link

cloudflare-workers-and-pages bot commented Mar 13, 2025

Deploying head-start with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1ef7394
Status: ✅  Deploy successful!
Preview URL: https://04c6e922.head-start.pages.dev
Branch Preview URL: https://256-add-groupingblock.head-start.pages.dev

View logs

Copy link
Author

@MarleenEliza MarleenEliza left a comment

Choose a reason for hiding this comment

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

@jurgenbelien Could you have a look at this PR? It is non breaking and add the GroupingBlock only

import GroupingBlock, { type Props } from './GroupingBlock.astro';

describe('GroupingBlock', () => {
test('renders a stack layout without a title when layout is "stack-untitled"', async () => {
Copy link
Author

Choose a reason for hiding this comment

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

Same layout tests as done previously on Page Partial

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not duplicate it. Remove the PagePartial tests that are about rendering it with a certain layout, and let the PagePartialBlock use the GroupingBlock to render.

Comment on lines 12 to 15
type GroupingItem = {
title: string;
blocks: AnyBlock[];
};
Copy link
Author

Choose a reason for hiding this comment

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

GroupingItem is only used inside GroupingBlock and does not exsist as a separate Block the content editor can add.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that you do have a Fragment for this that you can import...

@MarleenEliza MarleenEliza force-pushed the 256-add-groupingblock branch from c042538 to d6feeb6 Compare March 13, 2025 09:44
@MarleenEliza MarleenEliza changed the title add grouping block feature: add grouping block Mar 13, 2025
Copy link
Contributor

@jurgenbelien jurgenbelien left a comment

Choose a reason for hiding this comment

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

Let's make sure the grouping logic is only in grouping block, and PagePartial uses the logic in GroupingBlock to render it (for the time being)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can move this out of it's containing folder, and put the whole fragment definition inside of the other block's fragment file

import GroupingBlock, { type Props } from './GroupingBlock.astro';

describe('GroupingBlock', () => {
test('renders a stack layout without a title when layout is "stack-untitled"', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not duplicate it. Remove the PagePartial tests that are about rendering it with a certain layout, and let the PagePartialBlock use the GroupingBlock to render.

Comment on lines 12 to 15
type GroupingItem = {
title: string;
blocks: AnyBlock[];
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that you do have a Fragment for this that you can import...

Comment on lines +189 to +198
'BRbU6VwTRgmG5SbwUs0rBg',
'F60ZY1wFSW2qbvh99poj3g',
'PAk40zGjQJCcDXXPgygUrA',
'QYfZyBzIRWKxA1MinIR0aQ',
'TBuD6qQOSDy6i9dM3T_XEA',
'VZvVfu52RZK81WG0Dxp-FQ',
'V80liDVtRC-UYgd3Sm-dXg',
'ZdBokLsWRgKKjHrKeJzdpw',
'gezG9nO7SfaiWcWnp-HNqw',
'0SxYNS2CR1it_5LHYWuEQg',
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider writing this in a nondestructive way. See #245

For this migration I'd say do the following for pages and homepage:

const homePageBodyBlocks = (await client.fields.find("home_page::body_blocks"))
  .validators?.rich_text_blocks?.item_types || [];

await client.fields.update('pUj2PObgTyC-8X4lvZLMBA', {
  validators: {
    rich_text_blocks: {
      item_types: [
        ...homePageBodyBlocks,
        //  grouping block.

@MarleenEliza MarleenEliza force-pushed the 256-add-groupingblock branch from 9f2c445 to 1ef7394 Compare March 14, 2025 08:09
@jurgenbelien jurgenbelien self-requested a review March 14, 2025 10:24
Comment on lines +14 to +20
...block,
__typename: 'GroupingBlockRecord',
items: block.items.map(item => ({
title: item.title,
blocks: item.blocks as GroupingBlockFragment['items'][number]['blocks'],
__typename: 'GroupingItemBlockRecord'
}))
Copy link
Author

Choose a reason for hiding this comment

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

Not a very elegant way of typing, but mind that this is temporary

);

console.log(
'Create block model "\uD83D\uDDC2\uFE0F Grouping Item Block" (`grouping_item_block`)',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe name it GroupingItem instead of GroupingItemBlock? It should only be used inside the GroupingBlock, and should not be in AnyBlock.

block: GroupingBlockFragment
}
export type GroupingItem = GroupingItemBlockFragment & {
blocks: AnyBlock[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not expect AnyBlock here, the definition of blocks is in GroupItem(Block)Fragment

@@ -0,0 +1,12 @@
#import './GroupingItemBlock.fragment.graphql'
Copy link
Contributor

Choose a reason for hiding this comment

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

Paste the contents of this fragment underneath the GroupingBlock fragment

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.

2 participants