-
Notifications
You must be signed in to change notification settings - Fork 0
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
content modelling/865 fields in embed codes #17
Conversation
f3d635c
to
c958018
Compare
# | ||
# @return [string] A representation of the content block to be wrapped in the base_tag in | ||
# {#content} | ||
def content | ||
if field_names.present? | ||
content_for_fields |
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'm not sure the base presenter here should be processing field names, or if it should always only return title, but this was only way to test method to dig into nested fields worked without create a whole new type of block 🤔
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 think the way I planned it originally was to have a presenter for each block type, but if we're having a more field-based approach, this might not be the right move. I think this is fine for now - as we experiment, we can tidy this up a bit more.
we will need to use the embed code in order to see if a field name has been passed with it. Because a ContentBlockReference is returning the embed code already, which is what our consumer the Publishing API uses to assess embeds, then it will be easy to just pass it back when creating a new Content Block
we want to pass fields in in this format: {{embed:content_block_email_address:2593f06c-b7c8-4173-9a47-09d9bdd767ef/field_name/another_field}} so the regex query has to account for an optional `/` and then any number of characters before the closing `}}`
this adds a `content_for_fields` base method as default for all blocks - this will allow us to have a shared default handling of fields for block types.
this is a placeholder block type to help us test whether we can use field names in an embed code.
7adeb31
to
5240941
Compare
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 - just one tiny suggestion
<span | ||
class="content-embed content-embed__something" | ||
data-content-block="" | ||
data-document-type="something" |
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 wonder if we should add the field name to the data attributes? Might help with previewing
# | ||
# @return [string] A representation of the content block to be wrapped in the base_tag in | ||
# {#content} | ||
def content | ||
if field_names.present? | ||
content_for_fields |
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 think the way I planned it originally was to have a presenter for each block type, but if we're having a more field-based approach, this might not be the right move. I think this is fine for now - as we experiment, we can tidy this up a bit more.
09b45ac
to
67e0a7c
Compare
@@ -29,12 +29,15 @@ def render | |||
content_block: "", | |||
document_type: content_block.document_type, | |||
content_id: content_block.content_id, | |||
field_names: field_names.to_s, |
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.
Might be better to join
this with a comma, rather than presenting it as an array
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.
good shout, done!
to make it easier to find and track the blocks in published host documents.
67e0a7c
to
c0288f5
Compare
https://trello.com/c/UWUx0rCF/865-allow-individual-fields-to-be-embedded-with-embed-code
We want to enable users to pass field names via a Content Blocks embed code, like this:
{{embed:content_block_email_address:2593f06c-b7c8-4173-9a47-09d9bdd767ef/field_name/another_field}}
The field names currently can only be keys within a block's
details
json blob.This PR introduces:
embed_code
to be passed to a ContentBlock when presenting it, because we now need to know the content of the code to determine whether a field is required.I'm using the test type of postal address so we can see it working IRL.
next steps:
This repo is owned by the content modelling team. Please let us know in #govuk-publishing-content-modelling-dev when you raise any PRs.