Skip to content
This repository was archived by the owner on May 9, 2023. It is now read-only.

Adds top questions and links #453

Merged
allisonnorman merged 3 commits into from
Apr 8, 2020
Merged

Adds top questions and links #453

allisonnorman merged 3 commits into from
Apr 8, 2020

Conversation

@brentryanjohnson
Copy link
Contributor

@brentryanjohnson brentryanjohnson commented Apr 6, 2020

Fixes #451

Adds a top question to each promo box:

    - link: /protect-yourself/why-cloth-face-coverings
      question: Why do you need to wear cloth face coverings?

    - link: "/keeping-home-safe/if-someone-in-my-house-gets-sick"
      question: What should I do if someone in my house gets sick with COVID-19?

@brentryanjohnson
Copy link
Contributor Author

brentryanjohnson commented Apr 6, 2020

Re: CircleCI:

A test was written to fail if the promo boxes have more than two links in them. I'm not sure why that is, but that appears to be the origin of CircleCI failure:

 FAIL  test/index.html.test.js
 - both question boxes have two content links and a view all link

    expect(received).toHaveLength(expected)

    Expected length: 2
    Received length: 3

Mtcarp
Mtcarp approved these changes Apr 6, 2020
View reviewed changes
Copy link
Contributor

@Mtcarp Mtcarp left a comment

Choose a reason for hiding this comment

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

Looks good to me. While the other questions are written in first person, I understand the source content for face covering is not and it probably makes sense to keep the wording consistent.

@Mtcarp
Copy link
Contributor

Mtcarp commented Apr 6, 2020

Assuming the Circle CI fail is due to changes in how questions are linked?

@brentryanjohnson
Copy link
Contributor Author

brentryanjohnson commented Apr 6, 2020

It looks like the test was written to limit to two the number of questions in each box. I'm checking in with @adunkman about it.

adunkman
adunkman approved these changes Apr 6, 2020
View reviewed changes
Copy link
Contributor

@adunkman adunkman left a comment

Choose a reason for hiding this comment

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

👍 from an engineering perspective — with a note about updating the test name to reflect the new assertions.

test/index.html.test.js
boxes.forEach(({title, questions, viewAllText}) => {
expect(title).not.toEqual('');
expect(questions).toHaveLength(2);
expect(questions).toHaveLength(3);
Copy link
Contributor

@adunkman adunkman Apr 6, 2020

Choose a reason for hiding this comment

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

Don’t forget to update the name of this test! :)

@brentryanjohnson brentryanjohnson changed the title [WIP] Adds top questions and links Adds top questions and links Apr 6, 2020
@Mtcarp
Copy link
Contributor

Mtcarp commented Apr 6, 2020

@vbitzer Looks like this is ready for your review and approval to merge.

@allisonnorman allisonnorman self-requested a review April 8, 2020 16:27
@allisonnorman allisonnorman merged commit ed00f11 into dev Apr 8, 2020
@allisonnorman allisonnorman deleted the homepage-4-6 branch April 8, 2020 16:27
This was referenced Apr 10, 2020
Merged
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@vbitzer vbitzer Awaiting requested review from vbitzer

3 more reviewers

@adunkman adunkman adunkman approved these changes

@allisonnorman allisonnorman allisonnorman approved these changes

@Mtcarp Mtcarp Mtcarp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

(00) content

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update hompage boxes with facemask content

5 participants

@brentryanjohnson @Mtcarp @adunkman @allisonnorman