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: Details Block #181

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

feature: Details Block #181

wants to merge 8 commits into from

Conversation

luukbrauckmann
Copy link
Member

@luukbrauckmann luukbrauckmann commented Sep 18, 2024

Changes

  • Adds new Details Block

Associated issue

Resolves #171

How to test

  1. Open preview link
  2. Navigate to /en/demos/page-partial-layouts/
  3. You can also check Dato.

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

@luukbrauckmann luukbrauckmann linked an issue Sep 18, 2024 that may be closed by this pull request
3 tasks
@luukbrauckmann luukbrauckmann changed the title Created and added Details Block Feat: Details Block Sep 18, 2024
@luukbrauckmann
Copy link
Member Author

luukbrauckmann commented Sep 18, 2024

I'm a bit conflicted by the name DetailsBlock. If I would use this I would expect to render a details element, but maybe that's because I'm a developer who knows html. I suggest I rename it to DefinitionsBlock or DefinitionListBlock?

Also I added for now the | (pipe) character for separating multiple values. I could make this an option in which you can enter the split character in Dato, so the content editors can decide on a per block basis what they want to the split character to be. Does this sound like a good idea or overkill?

Copy link

cloudflare-workers-and-pages bot commented Sep 18, 2024

Deploying head-start with  Cloudflare Pages  Cloudflare Pages

Latest commit: 68fc8bf
Status: ✅  Deploy successful!
Preview URL: https://3fce157e.head-start.pages.dev
Branch Preview URL: https://feat-171-details-block.head-start.pages.dev

View logs

@luukbrauckmann luukbrauckmann marked this pull request as ready for review September 18, 2024 10:01
Copy link
Member

@jbmoelker jbmoelker left a comment

Choose a reason for hiding this comment

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

Nice work @luukbrauckmann! Left small comments.

src/blocks/Blocks.astro Outdated Show resolved Hide resolved
src/blocks/DetailsBlock/DetailsBlock.astro Outdated Show resolved Hide resolved
src/blocks/DetailsBlock/DetailsBlock.astro Outdated Show resolved Hide resolved
src/blocks/DetailsBlock/DetailsBlock.astro Outdated Show resolved Hide resolved
src/blocks/DetailsBlock/DetailsBlock.astro Outdated Show resolved Hide resolved
src/blocks/DetailsBlock/DetailsBlock.astro Outdated Show resolved Hide resolved
@@ -0,0 +1,4 @@
fragment DetailsBlock on DetailsBlockRecord {
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure to also include the migration for this block in the PR. We might be blocked by #164 by this 🤷 .

<>
<dt>{key}</dt>
{(value as string).split(splitCharacter).map((splitValue) => (
<dd>{splitValue}</dd>
Copy link
Member

Choose a reason for hiding this comment

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

I would also trim the value, so in the CMS editors can write "a | b | c" instead of "a|b|c", so splitValue.trim()

@jbmoelker jbmoelker changed the title Feat: Details Block feature: Details Block Sep 29, 2024
@jbmoelker
Copy link
Member

@luukbrauckmann you've already made some good iterations on this PR 🙌. As @decrek remarked and as we discussed, we could achieve this functionality as a variation on the Page Partial Block, rather than introducing a new one. We prefer that approach as it reduces the number of components we need to maintain and footprint we provide to developers using Head Start. What prompted the discussion and is another benefit to reusing the Page Partial Block, is that it allows for richer content in the detail list items (like text formatting and links).

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.

Details block
2 participants