-
Notifications
You must be signed in to change notification settings - Fork 194
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/659 Preview Host Documents via iFrame #9640
base: main
Are you sure you want to change the base?
Conversation
lib/engines/content_block_manager/app/services/content_block_manager/get_preview_content.rb
Fixed
Show resolved
Hide resolved
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 largely on the right track so far. Just a coupla suggestions 👍
lib/engines/content_block_manager/app/services/content_block_manager/get_preview_content.rb
Fixed
Show resolved
Hide resolved
41c0ea2
to
86fd2d5
Compare
lib/engines/content_block_manager/test/unit/app/services/get_preview_content_test.rb
Outdated
Show resolved
Hide resolved
lib/engines/content_block_manager/app/services/content_block_manager/get_preview_content.rb
Outdated
Show resolved
Hide resolved
1ec5d7d
to
13fbd46
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.
This is ace! Just a couple of things to think about. Feel free to push back on any / all of them if I'm barking up the wrong tree.
lib/engines/content_block_manager/test/unit/app/services/get_preview_content_test.rb
Outdated
Show resolved
Hide resolved
require "uri" | ||
|
||
module ContentBlockManager | ||
class GetPreviewContent |
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 this class could/should be more of a model-type class, especially as it returns something that looks like a model. I thinking something like:
class PreviewContent < Data.define(:title, :html)
class << self
def for_content_id(content_id)
PreviewContent.new(title: content_item["title"], html:)
end
private
def content_item
@content_item ||= begin
response = Services.publishing_api.get_content(@content_id)
response.parsed_content
end
end
def frontend_base_path
Rails.env.development? ? Plek.external_url_for("government-frontend") : Plek.website_root
end
def frontend_path
frontend_base_path + content_item["base_path"]
end
def html
uri = URI(frontend_path)
@html ||= Nokogiri::HTML.parse(Net::HTTP.get(uri))
end
end
end
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 one 👍 liking the use of Data
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've done this in a new refactor commit
lib/engines/content_block_manager/test/unit/app/services/get_preview_content_test.rb
Outdated
Show resolved
Hide resolved
<hr class="govuk-section-break govuk-!-margin-bottom-8 govuk-section-break--visible"> | ||
<div class="govuk-grid-row"> | ||
<div class="govuk-grid-column-full"> | ||
<iframe id="preview" style="width:100%;height:80vh;" srcdoc="<%= @preview_content[:html] %>"></iframe> |
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.
This is ace, I didn't know about srcdoc
!
lib/engines/content_block_manager/app/services/content_block_manager/get_preview_content.rb
Outdated
Show resolved
Hide resolved
lib/engines/content_block_manager/test/unit/app/services/get_preview_content_test.rb
Outdated
Show resolved
Hide resolved
def existing_content_block_spans(nokogiri_html) | ||
def style_blocks(nokogiri_html) | ||
content_block_spans(nokogiri_html).each do |span| | ||
span["style"] = "background-color: yellow;" |
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.
Would it be better to add a class here and then have the styling in the CSS? That way it'll be easier to make any tweakes further down the line - thinking maybe content-embed--preview
to stay true to the BEM way of doing things. Could alter this with something like:
span["style"] = "background-color: yellow;" | |
span["class"] = "#{span["class"]} content-embed--preview" |
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.
oh I meant to put something in the commit about this - i just hit a slight complication with adding a style to an iframe from outside of it, but I think it is possible
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.
Ah, course. Good point. Maybe if it gets tricky, we could at least have the style in a constant, so it's easier to spot.
|
||
def html | ||
uri = URI(frontend_path) | ||
@html ||= Nokogiri::HTML.parse(Net::HTTP.get(uri)) |
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.
Could/should we have a fallback if we don't get a response? I'm worried that when we run locally, we'll have to have the frontend stack running too, which could get annoying
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.
hmm that's a good point and definitely something that needs some design thinking - what do you think we should show for now? A paragraph element that says "Preview not found"?
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.
Yeah, I think that'll be enough for now
13fbd46
to
f654dd1
Compare
calls the gov.uk frontend to get the raw HTML of the page in the future we could find the embedded content block inside that HTML and transform it.
sp that we can use this id to get the preview HTML
here we iterate through the snapshot of HTML from the frontend, and find the `<span>` containing the existing edition of the Content Block we are previewing. We then replace it with the `render` output of the content block we are currently previewing.
f654dd1
to
6d5c5b8
Compare
We will need this Content ID on the Content Block Manager frontend, to get preview content here alphagov/whitehall#9640
We will need this Content ID on the Content Block Manager frontend, to get preview content here alphagov/whitehall#9640
a minimal styling to help us see the changed blocks in the build - TBC on actual UI design.
6d5c5b8
to
5a7cd8f
Compare
https://trello.com/c/e2rpWPUT/659-spike-preview-host-documents
We would like users to be able to click through from the edit journey to view a preview of the Host Document with the new Content Block content within it.
This PR suggests the first stage of enabling it via an iframe embedding the HTML from the gov.uk frontend of the Host Document.
We have come to this solution of using an iframe after extensive chats and found this to be the least bad option.
https://docs.google.com/document/d/19Nla6imjblR4XK_VI7t1lpob8Jmng2ZqRfpmsSnVuVQ/edit?tab=t.0#heading=h.s3ee1ypp6ehg
Follow these steps if you are doing a Rails upgrade.