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

DR-2574 Template for homepage #18

Merged
merged 6 commits into from
Oct 27, 2023
Merged

Conversation

7emansell
Copy link
Collaborator

@7emansell 7emansell commented Oct 25, 2023

Ticket:

Fixes JIRA ticket DR-2574

This PR does the following:

  • Adds the template to the DC homepage

How has this been tested?

Tested locally.

Accessibility concerns or updates

N/A.

Checklist:

  • I have added relevant accessibility documentation for this pull request.
  • All new and existing tests passed.

@7emansell 7emansell added the WIP Work in progress (don't merge) label Oct 25, 2023
@vercel
Copy link

vercel bot commented Oct 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
digital-collections ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 25, 2023 7:53pm

@7emansell 7emansell added the question Further information is requested label Oct 25, 2023
@@ -17,4 +17,15 @@ describe("Home", () => {
});
expect(header).toBeInTheDocument();
});

it("renders the expected text content", () => {
const { getByText } = render(<Home />);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Best I could come up with for unit tests on components that don't actually have any content, but is there a more thorough way to do this?

Copy link
Member

Choose a reason for hiding this comment

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

This is okay! We also had a hard time testing this on the DS. The accessibility test covers what we want as well.

return <CampaignHero />;
return (
<>
<SkipNavigation />
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this, I forgot about this. It also needs a bit more clarity from Clare or Lana about where the "Skip to Main Content" link should go to. The link goes to #mainContent but there's no element on the page with this id. Can you add it to the h1 in the Hero (currently has campaign-hero) for now?

We can bring this up once the swim lanes are complete -- should we point to the hero heading or the first swim lane?

return (
<>
<SkipNavigation />
<Template>
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, did you try the TemplateAppContainer component as well? You don't need to change it but just wondering if there were any issues with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refactored to use TemplateAppContainer

Copy link
Member

@EdwinGuzman EdwinGuzman left a comment

Choose a reason for hiding this comment

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

Just had one question about the TemplateAppContainer component and two minor updates.

<p>Rest of swim lanes</p>
<p>Explore further links</p>
</TemplateContent>
<TemplateFooter>
Copy link
Member

Choose a reason for hiding this comment

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

The NYPL Footer will go in a different location so this is okay to remove.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does this mean remove the template footer entirely?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, correct.

@7emansell 7emansell removed question Further information is requested WIP Work in progress (don't merge) labels Oct 25, 2023
@7emansell 7emansell merged commit 0ca6f2e into main Oct 27, 2023
3 checks passed
@avertrees avertrees deleted the feature/DR-2574/page-template branch July 11, 2024 16:23
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