-
Notifications
You must be signed in to change notification settings - Fork 198
Spike - simple child pages (shared content ID) [WHIT-3520] #11513
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?
Changes from 6 commits
3c4b972
3e78904
c26db21
f505d63
72dfa7a
bd18a63
284149f
de9919a
02ec9c0
f93db31
06fe704
c57b768
cfcca7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| module ConfigurableContentBlocks | ||
| class DefaultTextarea < BaseBlock | ||
| private | ||
|
|
||
| def template_name | ||
| "default_textarea" | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,36 @@ | |
| } | ||
| } | ||
| }, | ||
| "about": { | ||
| "dynamic": true, | ||
| "label": "About page", | ||
| "fields": { | ||
| "about_title": { | ||
| "title": "Title", | ||
| "block": "default_string", | ||
| "attribute_path": ["block_content", "about_title"], | ||
| "part": "/about", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd probably store the plain "about" here and inject the "/" where needed?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I find it odd that we have no mechanism to know where this block sits - like we shouldn't have to even do something like this. The tree traversal should give us this, and we should have the option to map this in the presenters section. Is it that our interpretation of Path has now veered towards leaf nodes so much? Like, if we had an attribute_path that had the block's title, we could use that. Something about you having to do this suggests the current implementation is lacking something. I just would not like to add extraordinary rules to what we already have, if possible.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm true - perhaps we can infer this by setting something like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There definitely seems to be a pattern/rule? of only using leaves in that attribute_path array now. We can explore some options when implementing this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in latest commits. |
||
| "part_name": "title", | ||
| "translatable": true | ||
| }, | ||
| "about_summary": { | ||
| "title": "Summary", | ||
| "block": "default_textarea", | ||
| "attribute_path": ["block_content", "about_summary"], | ||
| "part": "/about", | ||
| "part_name": "summary", | ||
| "translatable": true | ||
| }, | ||
| "about_body": { | ||
| "title": "Body", | ||
| "block": "govspeak", | ||
| "attribute_path": ["block_content", "about_body"], | ||
| "part": "/about", | ||
| "part_name": "body", | ||
| "translatable": true | ||
| } | ||
| } | ||
| }, | ||
| "social_media_accounts": { | ||
| "dynamic": true, | ||
| "label": "Social media accounts", | ||
|
|
@@ -128,6 +158,15 @@ | |
| "body": { | ||
| "type": "string" | ||
| }, | ||
| "about_title": { | ||
| "type": "string" | ||
| }, | ||
| "about_summary": { | ||
| "type": "string" | ||
| }, | ||
| "about_body": { | ||
| "type": "string" | ||
| }, | ||
| "social_media_links": { | ||
| "type": "array", | ||
| "attributes": { | ||
|
|
@@ -175,7 +214,8 @@ | |
| "publishing_api": { | ||
| "details": { | ||
| "body": "govspeak", | ||
| "social_media_links": "social_media_links" | ||
| "social_media_links": "social_media_links", | ||
| "parts": "parts" | ||
| }, | ||
| "links": [ | ||
| "organisations" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,12 @@ def social_media_links(attribute) | |
| } | ||
| end | ||
| end | ||
|
|
||
| def parts(_attribute) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know that I like injecting new flavours of things in the logic of these classes like this. The config driven approach becomes a bit un-hold-in-ones-head-able. If we expect the methods here to be mappers to keys that are attributes, let's try to follow that pattern.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Plus, wouldn't the rest of the presenter logic also loop through the about field and add it to the payload?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a case for an object block. Not sure whether we said we were doing those or not anymore 😅 It's a blur. Or alternatively we can add an array of fields with a single option. And then it is truly obvious how that would extend for multiple pages (though add another is not exactly great for this case).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
...no? By my understanding, everything in the presenter payload has to exist in the But to your first point - I agree. It would be much nicer if we could be more explicit in the presenter config, something like this: "presenters": {
"publishing_api": {
"details": {
"body": { "_source": "body", "_processor": "govspeak" },
"social_media_links": { "_source": "social_media_links", "_processor": "social_media_links" },
"parts": {
"_sources": {
"title": "{ "_source": "about_title", "_processor": "raw" },
"summary": "{ "_source": "about_summary", "_processor": "raw" },
"body": "{ "_source": "about_body", "_processor": "string" },
}
}
},
...
}
},That would mean we could drop the (Have gone for underscore "_source" notation in case for whatever reason you'd have a page with a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes yes "everything in the presenter payload has to exist..." - this is correct 🤦🏻♀️ I think what I meant with the "keys that are attributes" was that - most of those payload builder methods have the block_content calling that attribute.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this save like now, in the block_content? It doesn't wrap an "about" object over the fields, does it? 🤔 I imagine it would be block_content: { about_title: ..., about_summary etc}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have pushed up some commits changing the schema to use the default_object with an "about" namespace, rather than several flat "about_x" properties. Meant we can kill the |
||
| item.type_instance.parts.map do |part| | ||
| PayloadBuilder::MultipleParts.for(item, part) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| module PublishingApi | ||
| module PayloadBuilder | ||
| class MultipleParts | ||
| attr_reader :item, :part | ||
|
|
||
| def self.for(item, part) | ||
| new(item, part).call | ||
| end | ||
|
|
||
| def initialize(item, part) | ||
| @item = item | ||
| @part = part | ||
| end | ||
|
|
||
| def call | ||
| parts | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def parts | ||
| hash = { | ||
| slug: part.gsub("/", ""), | ||
| } | ||
| item.type_instance.fields_for_part(part).each_with_object(hash) do |field, obj| | ||
| obj[field["part_name"].to_sym] = item.block_content&.public_send(field["key"]) | ||
| end | ||
| hash | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| <%= render "govuk_publishing_components/components/textarea", { | ||
| textarea_id: block.path.form_control_id, | ||
| label: { | ||
| text: block.title + (block.required ? " (required)" : ""), | ||
| heading_size: "m", | ||
| }, | ||
| name: block.path.form_control_name, | ||
| value: block.value, | ||
| hint: block.hint_text, | ||
| right_to_left: block.edition.translation_locale.rtl?, | ||
| error_items: errors_for(block.edition.errors, block.path.validation_error_attribute.to_sym), | ||
| } %> | ||
| <% if block.primary_locale_value.present? %> | ||
| <%= render "govuk_publishing_components/components/details", { | ||
| title: "Primary locale content for #{block.title}", | ||
| } do %> | ||
| <%= block.primary_locale_value %> | ||
| <% end %> | ||
| <% end %> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| require "test_helper" | ||
|
|
||
| class ConfigurableContentBlocks::DefaultTextareaRenderingTest < ActionView::TestCase | ||
| include ConfigurableContentBlockSharedTests | ||
|
|
||
| setup do | ||
| @field = { | ||
| "block" => "default_textarea", | ||
| "title" => "Test attribute", | ||
| "description" => "A test attribute", | ||
| "attribute_path" => %w[block_content test_attribute], | ||
| "translatable" => true, | ||
| } | ||
| @path = Path.new(%w[block_content test_attribute]) | ||
| ConfigurableDocumentType.setup_test_types(build_configurable_document_type("test_type", { | ||
| "forms" => { | ||
| "documents" => { | ||
| "fields" => { | ||
| "test_attribute" => @field, | ||
| }, | ||
| }, | ||
| }, | ||
| "schema" => { | ||
| "attributes" => { | ||
| "test_attribute" => { | ||
| "type" => "string", | ||
| }, | ||
| }, | ||
| }, | ||
| })) | ||
| @edition = StandardEdition.new( | ||
| configurable_document_type: "test_type", | ||
| block_content: { "test_attribute" => "foo" }, | ||
| ) | ||
| @block = ConfigurableContentBlocks::DefaultTextarea.new(@edition, @field, @path) | ||
| end | ||
|
|
||
| test "the form label is equal to the attribute title" do | ||
| render @block | ||
| assert_dom "label", text: @field["title"] | ||
| end | ||
|
|
||
| test "it adds a required message to the label when the attribute is required" do | ||
| @field["required"] = true | ||
| render @block | ||
| assert_dom "label", text: "#{@field['title']} (required)" | ||
| end | ||
|
|
||
| test "it sets the textarea name correctly" do | ||
| render @block | ||
| assert_dom "textarea[name=?]", "edition[block_content][test_attribute]" | ||
| end | ||
|
|
||
| test "it sets the textarea value based on the content" do | ||
| render @block | ||
| assert_dom "textarea", text: @edition.block_content["test_attribute"] | ||
| end | ||
|
|
||
| test "it sets the hint text based on the description" do | ||
| render @block | ||
| assert_dom ".govuk-hint", text: @field["description"] | ||
| end | ||
|
|
||
| test "it sets the direction on the input to right to left when the current locale is Arabic" do | ||
| with_locale(:ar) do | ||
| render @block | ||
| end | ||
| assert_dom "textarea[dir=\"rtl\"]" | ||
| end | ||
|
|
||
| test "it renders the primary locale content under the textarea when the current locale is different from the primary locale" do | ||
| with_locale(:es) do | ||
| render @block | ||
| end | ||
|
|
||
| assert_dom ".govuk-details__text", text: @edition.block_content["test_attribute"] | ||
| end | ||
|
|
||
| test "it renders any validation errors when they are present" do | ||
| messages = %w[foo bar] | ||
| messages.each { |m| @edition.errors.add(:test_attribute, m) } | ||
|
|
||
| render @block | ||
| assert_dom ".govuk-error-message", "Error: #{messages.map { |m| "Test attribute #{m}" }.join}" | ||
| end | ||
| end |
Uh oh!
There was an error while loading. Please reload this page.
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.
It appears I cannot, in fact, imagine 😅
How would you extend this for multiple corporate information pages? Add all the types there as new forms, with their own fields, and part "/corporate-thing-blah"? 💀
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.
Yep, exactly! Or every corporate information page on one tab (like we have for Travel Advice pages):
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.
That's really busy 😵💫
The array solution might work better, and then you can just let the user add... another.
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.
Less bad in latest commits (each 'part' would be its own nested default_object, and there are fewer properties needed, e.g. part_name/part). But agree it's still not great.
As we touched on in 1:1 this morning, not entirely convinced this would scale to corporate information pages now I know CIP pages have an Attachments tab requirement too.
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.
Though could very well scale to Travel Advice 🤔