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

Add header and items list components #3

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

davidnhz
Copy link

@davidnhz davidnhz commented Feb 20, 2021

Adds Header component with elements such as Header and Items list to display mocked data for youtube videos.

Screen Shot 2021-02-19 at 9 09 06 PM

Copy link

@wdonet wdonet left a comment

Choose a reason for hiding this comment

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

Added a few comments but in general, it was good work 👍🏽
Notes:

  • The desktop view looks like a mobile view
  • The theme option is missing
  • Glad you could deploy it on github.io 👍🏽

import React from 'react';

import './Card.styles.css';
// import Styled from "./styled";
Copy link

Choose a reason for hiding this comment

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

let me know if you found issues applying the styled-components concept to your code

function Header() {
return (
<header className="header">
<div className="toolbar">
Copy link

Choose a reason for hiding this comment

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

This could be divided into smaller components thus you would have the code separate and isolated for testing and readability, thus the code here would look like:

<header>
   <ToolBar>
     <Menu />
     <Search />
   </ToolBar>
</header>

I can help with any kind of orientation at specific if you want

<div className="items-list" data-testid="items-list">
{items.map(({ etag, snippet }) => (
<Card
key={etag}
Copy link

Choose a reason for hiding this comment

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

This is in general fine, but if you're not sure the "etag" value would be unique, you can always use the array index as key

import mockedData from '../../youtube-videos-mock.json';

describe("items list", () => {
it("There is an items list displayed", () => {
Copy link

Choose a reason for hiding this comment

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

For the description of your test case, you could use test() instead of it() thus it can be read like:

"Test there is an items list displayed"

But it's a matter of one's perspective

const { items } = mockedData;
render(<List items={items} />);
expect(screen.queryByTestId("items-list").tagName).toBe('DIV');
expect(screen.queryAllByTestId("items-list").length).toBe(1);
Copy link

Choose a reason for hiding this comment

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

Doing the following you assert the existence of exactly one element and the content of it:

Suggested change
expect(screen.queryAllByTestId("items-list").length).toBe(1);
expect(screen.queryByTestId("items-list").innerHtml).toBe("any content");

it("There are 25 items in the items list", () => {
const { items } = mockedData;
render(<List items={items} />);
expect(screen.queryAllByTestId("item").length).toBe(25);
Copy link

Choose a reason for hiding this comment

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

with <List> you're rendering <Card>s and in consequence tags: <h4> and <img> which are reachable by role: heading and image respectively. It would be a more effective test if you assert against them and reserve the usage of test-id props to more complex or embedded not-easy-to-reach HTML elements

@@ -12,26 +17,25 @@ function HomePage() {
function deAuthenticate(event) {
event.preventDefault();
logout();
history.push('/');
history.push('/hola');
Copy link

Choose a reason for hiding this comment

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

Is this correct?

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