-
Notifications
You must be signed in to change notification settings - Fork 155
Refactor: Added CoreListItem block to fix repeating sublist issue #2041
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
Conversation
🦋 Changeset detectedLatest commit: 9dc0abd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📦 Next.js Bundle Analysis for @faustwp/getting-started-exampleThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
|
cc @justlevine |
colinmurphy
left a comment
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.
LGTM
.changeset/brown-games-jog.md
Outdated
| @@ -0,0 +1,52 @@ | |||
| --- | |||
| '@faustwp/blocks': major | |||
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 think this should be a major release. It should be a minor
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.
The thing is, after this change we'll need to include blocks.CoreListItem.fragments to the queries to be able to render the lists. Without that CoreList won't work. That's why I came up with major release. How should we proceed in this case?
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.
Would. existing users be affected when using ${blocks.CoreList.fragments.entry} only and not ${blocks. CoreListItem.fragments.entry}? if Yes then its a major release.
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, existing users will stop seeing the list items if they just use ${blocks.CoreList.fragments.entry}.
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.
You could include the ... on CoreListItem {} fragment inside CoreList.fragments.entry for backwards compatibility. It would just get merged with whatever is defined separately on CoreListItem.fragments.entry.
I'm not entirely sure how the overloading DX works for y'all from a SemVer POV though: Is there a reason a user would be overloading CoreList.fragments.entry (and therefore not get the ...on CoreListItem frag(s) ) but not overloading the CoreList React component (and therefore need the CoreListItem component and a frag to populate it)?
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.
Thank you @justlevine for your suggestion. As we're using flatListToHierarchical on client side to build innerBlocks from editorBlocks flat list, it prevents us to benefit CoreListItem fragment inside CoreList.
Thus, we're intending to achieve backward compatibility by rendering the parsed CoreList attribute.values for those who don't add CoreListItem fragments temporarily and fully switching to CoreListItem with the first major release.
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 sure I understand the relevance of innerBlocks here, but if y'all have a solution that maintains backwards compat and stops this from beaking a breaking change, then no need for me to wallflower 🚀
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 was thinking something like this:
CoreList.fragments = {
key: `CoreListBlockFragment`,
entry: gql`
fragment CoreListBlockFragment on CoreList {
attributes {
anchor
backgroundColor
className
fontFamily
fontSize
gradient
lock
ordered
reversed
start
style
textColor
type
values
cssClassName
}
innerBlocks {
... on CoreListItem {
attributes {
content
anchor
backgroundColor
borderColor
className
fontFamily
fontSize
gradient
lock
metadata
placeholder
style
textColor
}
}
}
}
`,
};@justlevine do you think this is the misinterpreted version of your suggestion? 😅
Update: I think this is the correct one, isn't it?
CoreList.fragments = {
key: `CoreListBlockFragment ...CoreListItemFragment`,
entry: gql`
fragment CoreListBlockFragment on CoreList {
attributes {
anchor
backgroundColor
className
fontFamily
fontSize
gradient
lock
ordered
reversed
start
style
textColor
type
values
cssClassName
}
}
fragment CoreListItemFragment on CoreListItem {
attributes {
content
anchor
backgroundColor
borderColor
className
fontFamily
fontSize
gradient
lock
metadata
placeholder
style
textColor
}
}
`,
};
|
|
||
| Example query: | ||
|
|
||
| ```javascript |
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.
Great docs for the CHANGELOG 👏
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.
Thanks @colinmurphy 🙂
|
Released in the latest version |

Tasks
Description
CoreList was rendering values attribute, which happens to return nested list items multiple times. New CoreListItem block fixes this issue by recursively rendering nested lists.
Related Issue(s):
Testing
singlecomponent towp-templatesfolder with the component below. This component has CoreListItem fragments (${blocks.CoreListItem.fragments.entry}) to support CoreListItemhttp://localhost:3000/{your-post-slug}Screenshots
Documentation Changes
N/A
Dependant PRs
N/A