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

feature: callouts audit [NP-2114] #1213

Merged
merged 6 commits into from
Jun 14, 2021
Merged

Conversation

luke-hill
Copy link
Contributor

  • Adds in 2 new scenarios for rendering rules
  • Uses some clever new SitePrism inheritance to check for presence / overloaded classes

Also fixed up some leftover code regarding APP_ENVIRONMENT vs TESTING_BASE_URL

Copy link
Contributor

@davidsauntson davidsauntson left a comment

Choose a reason for hiding this comment

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

Small change about the nest heading level checks - we aren't really testing anything here since it's hard coded and the code to test is in the public website repo (and is not limited to callouts but handles all heading levels generally).

@@ -29,3 +37,23 @@
Then("a/an {string} label is present above the callout title") do |label|
expect(@component.label.text).to eq(I18n.t("cads.callout.#{label.downcase}"))
end

Then("both callouts are rendered correctly") do
expect(@component.outer_standard).to be_all_there(recursion: :one)
Copy link
Contributor

Choose a reason for hiding this comment

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

what does the be_all_there method test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that validates the presence of either
a) All defined SitePrism constructs within your scope
b) All permissible SitePrism constructs within your scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK thanks - so in this case the inner_adviser falls into (b) above because its section definition is nested inside the one for outer_standard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry I forgot to explain another tiny little bit, but you're basically correct.

I have got a long-standing piece of work to also allow recursion. So the way you detect if it's a or b is if there is a DSL definition of expected_items that will "filter" and only check for certain things.

outer_standard.all_there? => Checks outer_standard.heading and outer_standard.message
outer_standard.all_there?(recursion: :one) => Checks all of the above, then finds all section and sections definitions. For each of those, perform item.all_there? So in this instance it will check outer_standard.inner_adviser.heading outer_standard.inner_adviser.message and outer_standard.inner_adviser.label

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, thanks for the info!

I wonder if it's easier to just be explicit about what you're expecting and have two be_present calls, one for each callout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could do that if you'd prefer. In this situation that's not a problem because the items being enumerated are singular. So I can do it in 2.

But if the item being enumerated was plural, it could be say 10 items (Then it would be quite impractical).

Is there something language wise that's tricky to understand with the all_there assertion?

Copy link
Contributor Author

@luke-hill luke-hill Jun 10, 2021

Choose a reason for hiding this comment

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

To clarify would you prefer either

expect(a).to be_all_there(recursion: :one)
expect(a).to be_all_there

expect(b).to be_all_there

or

expect(a).to have_xxx

expect(a).to have_yyy

expect(b).to have_xxx
... and so on...

Note the latter is going to create a lot of overhead

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this - no major problem now I know 😄

Given there are Nested Callout components on the page
Then both callouts are rendered correctly

Scenario: Callouts must not break WCAG heading guidelines
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no code in the design system that changes the heading levels - it's all done in the public website here: https://github.com/citizensadvice/public-website/blob/develop/app/lib/formatters/heading_formatter.rb

The tests for this code should be in that repo (there are already spec tests).

This variant is just here for the VRTs and docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove this scenario.

Copy link
Contributor

@davidsauntson davidsauntson left a comment

Choose a reason for hiding this comment

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

👍🏻

@luke-hill luke-hill merged commit ef91e72 into master Jun 14, 2021
@luke-hill luke-hill deleted the feature/np-2114-callouts-audit branch June 14, 2021 08:47
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.

2 participants