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

Display content lists on publications irrespective off h2 count #3361

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

CodeSonia
Copy link
Contributor

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3361 October 11, 2024 09:51 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3361 October 11, 2024 10:58 Inactive
@hannako hannako force-pushed the display-content-lists-on-publications-irrespective-off-h2-count branch from bb4e984 to 872a756 Compare October 14, 2024 13:26
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3361 October 14, 2024 13:26 Inactive
@hannako hannako force-pushed the display-content-lists-on-publications-irrespective-off-h2-count branch from 872a756 to 006201a Compare October 14, 2024 13:28
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3361 October 14, 2024 13:28 Inactive
Copy link
Contributor

@hannako hannako left a comment

Choose a reason for hiding this comment

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

Happy to pair on this Sonia! But thought I'd type it out so you can work through it in your own time if you prefer.

@@ -19,14 +19,11 @@ def contents_items
end

def show_contents_list?
Copy link
Contributor

@hannako hannako Oct 15, 2024

Choose a reason for hiding this comment

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

Once #3366 is merged and you rebase, what we need here is:

return true if contents_items.present?
return false if no_first_item?

first_item_size_requirements_met?(character_count, table_row_count)

The behaviour that we want is if there are any contents_items at all (which are basically h2's) we want show_contents_list? to return true. This is why we have the return keyword. It returns true and exits the method so the subsequent lines aren't run (we don't care about the first item if it's got an H2). It's explained well on here.

If there are no contents_items ie no H2's then we will look at the first_item, ie the first chunk of content in the body. It has to meet certain requirements, and there's logic to figure that out. But there's no point spending time running that logic if there's no first_item, so we use the Explicit return statement again... ie if there's no_first_item? we should return false and exit.

And finally, if there is a first_item, then we check if it meets various character count requirements. We don't need the return keyword here, as the last thing in a method is returned (Implicit return).

test/integration/specialist_document_test.rb Show resolved Hide resolved
test/integration/working_group_test.rb Show resolved Hide resolved
@@ -84,12 +84,11 @@ def body
@contents_list.contents_items
end

test "#show_contents_list? returns true if number of contents items is less than 3 and the first item's character count is above 415" do
test "#show_contents_list? returns true if the first item's character count is above 415" do
Copy link
Contributor

Choose a reason for hiding this comment

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

This file needs to have the following test cases:

  1. Displays contents list if there is an H2
def body
  "<h2 id='one'>One</h2>"
end
  1. Displays contents list if there is no H2 but first item is greater than 415
def body
  "<div><p>#{Faker::Lorem.characters(number: 416)}</p></div>"
end
  1. Displays no contents list if there is no H2 and first item is less than 415
def body
  "<div><p>#{Faker::Lorem.characters(number: 400)}</p></div>"
end

And then the tests for the other cases (images and tables)

I think all of them are present in this file, but you just need to tweak them so that the body provides the right structure. Hope that makes sense!

@CodeSonia
Copy link
Contributor Author

Happy to pair on this Sonia! But thought I'd type it out so you can work through it in your own time if you prefer.

Thanks Jess, this is great! I'll have a look and make a start 👀

@CodeSonia CodeSonia force-pushed the display-content-lists-on-publications-irrespective-off-h2-count branch from 006201a to 0a9bcb4 Compare October 16, 2024 10:17
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3361 October 16, 2024 10:17 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3361 October 16, 2024 10:18 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3361 October 16, 2024 15:31 Inactive
@CodeSonia CodeSonia force-pushed the display-content-lists-on-publications-irrespective-off-h2-count branch from 561ec2b to a392585 Compare October 16, 2024 15:43
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3361 October 16, 2024 15:43 Inactive
Updated tests that checked for no content list with fewer than three headings to align with the new logic in the show_contents_list? method in contents_list.
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.

3 participants