-
Notifications
You must be signed in to change notification settings - Fork 1
feature: Heading Block #267
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
base: main
Are you sure you want to change the base?
Conversation
Deploying head-start with
|
| Latest commit: |
6f016ef
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4def7ebe.head-start.pages.dev |
| Branch Preview URL: | https://feat--heading-block.head-start.pages.dev |
jurgenbelien
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.
Looks good! Great that you've included the tests and included in Home, Pages, PagePartials and TextBlocks. Could you also add it to the TextImageBlock Structured Text?
The individual comments are mostly regarding code style, to align it a little more with how other components and blocks are written.
|
|
||
| const { block } = Astro.props; | ||
|
|
||
| const Element = `h${block.level || 2}`; |
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.
Consider renaming this to Tag to align with the implementations in nodes/Heading.astro in TextBlock and StructuredText. It also differentiates more explicitly string-based html tags from dynamic Astro components.
231bd8d to
819db18
Compare
MarleenEliza
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.
Still some references to subtitle, but other than that looks good! Similar to our implementation in Zzuper
819db18 to
bae61e1
Compare
heading -> title sub_heading -> subheading
bae61e1 to
8f21000
Compare
MarleenEliza
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.
Confirmed it's working and code looks good!
Only last thing I noticed is that currently the custom text pluigin is not compatible witht he Heading Block. If this is by choice, I will approve the PR
123e534 to
4a45777
Compare
4a45777 to
6a8c204
Compare
| console.log( | ||
| 'Create Structured text field "Text" (`text`) in block model "#\uFE0F\u20E3 Heading Block" (`heading_block`)' | ||
| ); | ||
| const headingBlockText = await client.fields.create(headingBlock.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.
Can we add a clear helper text here? It should be clear to the editor what this field is for and what the 'level' field does.
|
@petergoes @jurgenbelien @MarleenEliza @sjoerdbeentjes I'm late to the party 😊, so maybe this has been discussed by you already: Since we('ll) now have a "custom text plugin" I was wondering if that plugin could take away the possible confusion of the h2-h6 selection in the Structured Text field. Then everything could be a paragraph and we add the custom text styles for the different appearances. And the |
Add HeadingBlock
Closing #265
Renders H2-H6 elements
Features
levelfield for numbers between 2 and 6