Create a Card block content type #76

phenaproxima · 2024-05-15T20:54:25Z

This is part of #12.

In a one-on-one chat, @larowlan and I felt that it would be a good idea to start adding some useful block types, even though we don't quite have a page layout system in Starshot yet. A card block type seems useful because, even if we don't have a decent layout system, cards might still make sense in the sidebar in some situations. There's no harm in building out the data model/fields now, even without any special affordance for layout.

simesy

Semantic/reuse modelling review. This PR highlights two fields that (I would love if) Core somehow shipped opinionated versions of: a CTA link field, and a mixed media hero field.

simesy · 2024-05-15T21:11:02Z

composer.json

@@ -106,7 +106,8 @@
        "patches": {
            "drupal/core": {
                "#3444246: Create a config action to set the status of the entity": "./patches/core/recipes-103.patch",
-                "Add config actions to make a bundle of an entity type translatable": "./patches/core/make-translatable-action.patch"
+                "Add config actions to make a bundle of an entity type translatable": "./patches/core/make-translatable-action.patch",
+                "Give block content entities bundle-level permissions granularity": "./patches/core/block-content-granularity.patch"


Should patches like this reference a drupal.org issue?

Yes, but this one has no d.o issue. I just spun up the patch quickly.

If there is an existing issue with a patch (or you want to create one), let's use that.

Is there a core issue for this, feels like an oversight given we do have bundle perms for them since drupal.org/node/1975064

Yes, but this one has no d.o issue.

So if there is no #123456 in the patch description, then it is owned by the prototype (until it's not).

More or less, yeah. The rules are loose; patches are completely internal, so we can name them however we want.

recipes/starshot_card_block_type/config/core.entity_form_display.block_content.card.default.yml

simesy · 2024-05-15T21:13:43Z

recipes/starshot_card_block_type/config/field.field.block_content.card.body.yml

+  module:
+    - text
+id: block_content.card.body
+field_name: body


I think if you're going to use field_ elsewhere then use it for everything, as other than being persistent "body" is just a field and shouldn't have special treatment.

Except that we're using the body field that ships with the block_content module, so it's kind of an inherited decision.

I don't feel super strongly but I'd rather not create new fields when we can reuse them.

You can just delete the body field, it doesn't break anything. We shouldn't be limited by old core decisions of which this one isn't great.

Note that i advocate no field_ prefixing, which would make body good. https://www.youtube.com/watch?v=2495KafRZAg

I don't think we really need to get too hung up on the fields' internal names.

Starshot has no update path, so we can always change this later, whenever we want. I think it's safe to assume that end users probably won't care about the names; since Starshot is meant to put users first, I think it makes sense to merge this if the data model overall makes sense, and we can bikeshed the field names in a separate PR.

We have to be mindful that decisions made 10 years set what people think is best practice. So we have to start a dialog about what we're pushing out (and have a way to defer gold plating without dropping the ball). I really would love to see a target content model done in Excalidraw or something so that we can measure PRs against it.

I'm in favor of developing some reasonable guidelines around field names, but this repository is a rapid prototype of Starshot developed mostly by fiat, with the goal of delivering as much value to end users as we can, as quickly as we can. So we can play a little fast and loose, at least for now. :)

simesy · 2024-05-15T21:16:20Z

recipes/starshot_card_block_type/config/field.field.block_content.card.field_card_link.yml

+dependencies:
+  config:
+    - block_content.type.card
+    - field.storage.block_content.field_card_link


here "field_card_link" appears to have the intention of offering a call to action. This is a very generic thing and I would prefer to see a "field_ctalink" or field_cta_link or something like that can be reused - and hence if, for example, it is themed it is theme consistently. (Bearing in mind I don't yet know how well recipes will overlap with shared fields.)

No objection to renaming the field. The machine names of these fields won't matter much to end users, and we can rename them whenever we want since there is no update path.

Personally I go with a generic 'link' if singular and 'links' if plural for maximum re-use and minimum number of DB tables.

simesy · 2024-05-15T21:17:54Z

recipes/starshot_card_block_type/config/field.field.block_content.card.field_card_media.yml

+    - media.type.remote_video
+    - media.type.video
+id: block_content.card.field_card_media
+field_name: field_card_media


Same comment as with link... Should it be reused? and media is singular and plural. The intention seems to be a single media item for displaying an image or a video. I often choose "hero" for this which is a very common and meaningful way to describe a single media instance that is canonical for an entity.

I'm okay with renaming this to field_hero.

I don't know that hero is correct here given there will be multiple cards in a card grid

Echoing #76 (comment), I'd recommend we not get too deep into the weeds of how the fields are named -- at least not in this PR.

larowlan

This looks good, I think we should also think about a 'card reference' block type recipe (Separate PR) that would add the following:

new block type with a single ER field to a node
new card image field on all node types
new card teaser field on all node types
form display changes to put those in a details element in the sidebar on the node form
card view mode for all node types that used the node title, url, image, teaser to replicate the card block content type we're adding here

larowlan · 2024-05-15T21:23:13Z

composer.json

@@ -106,7 +106,8 @@
        "patches": {
            "drupal/core": {
                "#3444246: Create a config action to set the status of the entity": "./patches/core/recipes-103.patch",
-                "Add config actions to make a bundle of an entity type translatable": "./patches/core/make-translatable-action.patch"
+                "Add config actions to make a bundle of an entity type translatable": "./patches/core/make-translatable-action.patch",
+                "Give block content entities bundle-level permissions granularity": "./patches/core/block-content-granularity.patch"


Is there a core issue for this, feels like an oversight given we do have bundle perms for them since drupal.org/node/1975064

recipes/starshot_card_block_type/config/core.entity_form_display.block_content.card.default.yml

recipes/starshot_card_block_type/config/core.entity_view_display.block_content.card.default.yml

larowlan · 2024-05-15T21:27:16Z

recipes/starshot_card_block_type/config/field.field.block_content.card.field_card_link.yml

+dependencies:
+  config:
+    - block_content.type.card
+    - field.storage.block_content.field_card_link


Personally I go with a generic 'link' if singular and 'links' if plural for maximum re-use and minimum number of DB tables.

larowlan · 2024-05-15T21:28:48Z

recipes/starshot_card_block_type/config/field.field.block_content.card.field_card_media.yml

+    - media.type.remote_video
+    - media.type.video
+id: block_content.card.field_card_media
+field_name: field_card_media


I don't know that hero is correct here given there will be multiple cards in a card grid

phenaproxima · 2024-05-16T00:17:16Z

I think we should also think about a 'card reference' block type recipe (Separate PR)

@larowlan, can you open a separate issue or PR for that?

phenaproxima · 2024-05-21T11:33:26Z

As of #77, I think we might want to start using Type Tray for the block types, and ship some icons.

phenaproxima added 4 commits May 15, 2024 16:50

Create a Card block content type.

44fd6aa

Fix permissions

dee6f4b

Patch core for bundle-level permissions on block content.

e713c8d

Make Body field of card blocks smaller

105155f

simesy reviewed May 15, 2024

View reviewed changes

larowlan reviewed May 15, 2024

View reviewed changes

phenaproxima added 5 commits May 15, 2024 20:01

Hide field labels

6808e62

Add card view mode for media

2cee826

Add base field override for block info field

8afad32

Add separate title field to card blocks

50b4f1a

Make all fields translatable by default

0be6973

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a Card block content type #76

Create a Card block content type #76

phenaproxima commented May 15, 2024

simesy left a comment •

edited

simesy May 15, 2024

phenaproxima May 15, 2024

larowlan May 15, 2024

simesy May 15, 2024 •

edited

phenaproxima May 16, 2024

simesy May 15, 2024

phenaproxima May 15, 2024 •

edited

simesy May 15, 2024

phenaproxima May 15, 2024

simesy May 16, 2024

phenaproxima May 16, 2024 •

edited

simesy May 15, 2024

phenaproxima May 15, 2024

larowlan May 15, 2024

simesy May 15, 2024

phenaproxima May 15, 2024

larowlan May 15, 2024

phenaproxima May 15, 2024

larowlan left a comment

larowlan May 15, 2024

larowlan May 15, 2024

larowlan May 15, 2024

phenaproxima commented May 16, 2024

phenaproxima commented May 21, 2024

Create a Card block content type #76

Are you sure you want to change the base?

Create a Card block content type #76

Conversation

phenaproxima commented May 15, 2024

simesy left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simesy May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phenaproxima May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phenaproxima May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larowlan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phenaproxima commented May 16, 2024

phenaproxima commented May 21, 2024

simesy left a comment •

edited

simesy May 15, 2024 •

edited

phenaproxima May 15, 2024 •

edited

phenaproxima May 16, 2024 •

edited