Mini ch 3 #52

cylcrow · 2021-03-08T20:15:12Z

What this PR do?

Performs tasks specified for Mini challenge 3

Any background context you want to provide?

Add your google API Key in a file called .env.development under the name of REACT_APP_YOUTUBE_KEY

Notes:

There are uncovered components for testing, sorry... I wanted to make my PR sooner, I'll cover them in a later revision.
I added a prototype for theming, is not finished, do not expect too much.

Where should the reviewer start?

Well... I'm not sure, it was a really big refactor and new features.

How should this be manually tested?

running tests

Screenshots

UI:

Theming prototype:

Coverage:

- Removed unnecessary files - Added first tests - Added first components - Added dependencies for testing

…not yet created components

…cription

- First iteration for base components - Modified Header structure: created StyledSection component to a better arrangement of components

- Added card component - Added HomeVideos component - Added tests for new added components - Added/refactored styles for HomeVideo and Card component - Refactored tests for Card component

…ests refactor to comply just with react-testing-library

- Added roles to elements to improve accesibility - Refactored tests to find elements by role instead of their id

- ✨ Added theming prototype - ✅ Added first specs to test integration with theming

- Changed directory name 'Base' to 'ui', indicating that files contained here will be for general UI integration - Updated tests in Card component

- Refactored code - Moved mocked files to a new folder - Mocked API calls - Created tests for custom hooks

- Added test for YT playback frame - Added component for youtube playback

- Added SmallVideoCard component - Added RelatedVideoList component - Added styles for each new component - Added tests for each new component

- Removed unnecesary code from VideoPlayerContainer - Added tests to assert small captions video change - Improved code format

jparciga

Mini-Challenge 3 Feedback:

Acceptance Criteria

✅ Videos in the Home View are fetched from the YouTube API and displayed correctly.
✅ Users can search for YouTube videos using the search field on the header.
✅ A video can be played from the Video Details View after clicking on it.
✅ The video information and related videos list are displayed correctly on the Video Details View: Please add the title and description for the selected video on the Video Details View.
✅ When a user clicks on a related video the video player, information, and related videos list are updated.

Bonus Points

✅ Custom Hooks for API calls are implemented and tested correctly.
✅ Testing coverage is above 60%. (Please include a screenshot of the code coverage report in your PR description).

Improvements

Whenever you have time, it'll be great to apply media queries to have a responsive design

For some reason the video player size is looking weird

Sometimes when I click on a related video the following error appears:

jparciga · 2021-03-17T18:34:16Z

src/components/HomeVideos/HomeVideos.spec.jsx

+  const contextValue = { search: jest.fn(), videos: youtubeMockedData.items };
+  const Wrap = contextWrapper(SearchContext, contextValue, Component);
+  const { container } = renderWithTheme(Wrap, theme);
+  return { container };


That's a nice way to mock the context value 👌🏻

jparciga · 2021-03-17T18:39:16Z

src/components/HomeVideos/VideoCard.test.jsx

+  it('shows content complying with ARIA attributes', () => {
+    const video = youtubeMockedData.items[0];
+    const { container } = build(<VideoCard video={video} />);
+
+    const img = getByRole(container, 'img');
+    const figcaption = container.querySelector('figcaption');
+
+    expect(img).toHaveAttribute('alt', video.snippet.title);
+    expect(img).toHaveAttribute('src', video.snippet.thumbnails.high.url);
+    expect(figcaption).toHaveTextContent(video.snippet.description);
+
+    expect(getByTitle(container, 'video-title')).toHaveTextContent(video.snippet.title);
+    expect(container).toHaveTextContent(video.snippet.description);
+  });


Nice test case!

jparciga · 2021-03-17T18:47:45Z

src/components/Layout/Header.spec.jsx

+  it('fires user search until presses enter', () => {
+    const EXPECTED_TEXT = 'Hello, ';
+    const TRIGGER_TYPING = 'there';
+    const { searchInput, contextValue } = build();
+    fireEvent.change(searchInput(), { target: { value: TRIGGER_TYPING } });
+    fireEvent.change(searchInput(), { target: { value: EXPECTED_TEXT } });
+    expect(searchInput().value).toBe(EXPECTED_TEXT);
+    expect(contextValue.search).not.toHaveBeenCalled();
+    fireEvent.keyPress(searchInput(), { key: 'Enter', code: 13, charCode: 13 });
+    expect(contextValue.search).toHaveBeenCalled();
+  });


Excellent 👏🏻

jparciga · 2021-03-17T18:56:17Z

src/components/Layout/LayoutWrapper.jsx

+const StyledLayoutWrapper = styled.div`
+  background: ${({ theme }) => theme.color.background};
+  width: 100%;
+  height: 100%;
+`;
+
+const LayoutWrapper = ({ children }) => {
+  const { theme } = useContext(ThemeContext);
+
+  return (
+    <StyledLayoutWrapper data-testid="layout-wrapper" theme={theme} role="application">


It's not necessary to get the current theme from the context so your styled-components can use it. Since you're wrapping your App using the ThemeProvider all your styled-components will automatically receive the current theme in their props. https://styled-components.com/docs/advanced

Suggested change

const StyledLayoutWrapper = styled.div`

background: ${({ theme }) => theme.color.background};

width: 100%;

height: 100%;

`;

const LayoutWrapper = ({ children }) => {

const { theme } = useContext(ThemeContext);

return (

<StyledLayoutWrapper data-testid="layout-wrapper" theme={theme} role="application">

const StyledLayoutWrapper = styled.div`

background: ${({ theme }) => theme.color.background};

width: 100%;

height: 100%;

`;

const LayoutWrapper = ({ children }) => {

return (

<StyledLayoutWrapper data-testid="layout-wrapper" role="application">

jparciga · 2021-03-17T19:04:48Z

src/components/VideoPlayer/VideoPlayerContainer.jsx

+  useEffect(() => {
+    /* global YT */
+    /* eslint no-undef: "error" */
+      window.player = new YT.Player('player', {
+        height: '50%', 
+        width: '60%',
+        videoId,
+      });
+  });


It might be a good idea to add the videoId as a dependency for this useEffect so this code its executed only when the videoId changes.

Suggested change

useEffect(() => {

/* global YT */

/* eslint no-undef: "error" */

window.player = new YT.Player('player', {

height: '50%',

width: '60%',

videoId,

});

});

useEffect(() => {

/* global YT */

/* eslint no-undef: "error" */

window.player = new YT.Player('player', {

height: '50%',

width: '60%',

videoId,

});

}, [videoId]);

jparciga · 2021-03-17T19:26:09Z

src/utils/testing/helpers/renderWithTheme.jsx

+import React from 'react';
+import { render } from '@testing-library/react';
+import { ThemeProvider } from 'styled-components';
+import { lightTheme } from '../../../providers/themes';
+
+const renderWithTheme = (Component, theme = lightTheme) => {
+  const contextValue = { theme, switchTheme: jest.fn() };
+  return render(<ThemeProvider theme={contextValue}>{Component}</ThemeProvider>);
+};
+
+export default renderWithTheme;


Nice util 👍🏻

jparciga · 2021-03-17T19:27:18Z

src/utils/testing/mocks/googleMockedAPIObject.js

+import youtubeMockedData from './youtubeMockedData';
+
+const errorObject = {
+  error: {
+    code: 403,
+    message: '',
+    errors: [
+      {
+        message: '',
+        domain: '',
+        reason: '',
+      },
+    ],
+  },
+};
+const mockedGoogleAPIObject = (shouldResolveToSuccess = true) => ({
+  load: jest.fn(),
+  client: {
+    request: jest.fn().mockReturnValue(
+      new Promise((res, rej) => {
+        if (shouldResolveToSuccess) {
+          res({ result: youtubeMockedData });
+        }
+        rej(errorObject);
+      })
+    ),
+  },
+});
+
+export default mockedGoogleAPIObject;


cylcrow and others added 19 commits February 19, 2021 16:16

✅ ➕ 🎨 🔥 Fork, cleanup and first tests and component's creation

a8b259f

- Removed unnecessary files - Added first tests - Added first components - Added dependencies for testing

🚧 ✅ 🎨 Added tests to assert components default styling and stubs for …

6466b34

…not yet created components

♻️ Refactored names to comply with the specified in the challenge des…

dca62d2

…cription

♻️ ✅ ✨ 💄 Added base components located in Header component

07be5b1

- First iteration for base components - Modified Header structure: created StyledSection component to a better arrangement of components

✨ ♻️ ✅ 💄 Added Card component

d48c4e1

✨ ♻️ ✅ 💄 Added Card component

88e5f76

- Added card component - Added HomeVideos component - Added tests for new added components - Added/refactored styles for HomeVideo and Card component - Refactored tests for Card component

💄 🎨 Polished styles for Card component

a3c7089

testing-libraries-cleanup: ♻️ Removed unnecesary dependencies after t…

f7b8cf8

…ests refactor to comply just with react-testing-library

🍱 ♿ ♻️ Added roles to each element

e5c9eac

- Added roles to elements to improve accesibility - Refactored tests to find elements by role instead of their id

✅ ♻️ ✨ Added theming prototype

4547dca

- ✨ Added theming prototype - ✅ Added first specs to test integration with theming

🚧 ♻️ Added theme wrapper for testing purposes

436964f

♻️ 🏗️ 🚧 Components isolation

991f0b5

- Changed directory name 'Base' to 'ui', indicating that files contained here will be for general UI integration - Updated tests in Card component

♻️ ✨ ♿ ⚗️ Added VideoCards

be967d3

♻️ ♿ 🚧 Improved accesibility for Card components

7ed4920

✨ 🔥 🚚 ♻️ 🎨 🐛 ✅ Added youtube API

671b550

- Refactored code - Moved mocked files to a new folder - Mocked API calls - Created tests for custom hooks

♻️ 🎨 ✨ 🚚 Adding youtube playback iframe

233f180

- Added test for YT playback frame - Added component for youtube playback

♻️ 🐛 ✨ ✅ Added video player and video change

0fb5003

- Added SmallVideoCard component - Added RelatedVideoList component - Added styles for each new component - Added tests for each new component

✨ 🐛 🚧 Added VideoPlayer feature to return to HomeVideos on search

c1c945a

✅ 🔥 🎨 Added remaining tests for mini ch 3

d3123dc

- Removed unnecesary code from VideoPlayerContainer - Added tests to assert small captions video change - Improved code format

cylcrow force-pushed the mini-ch-3 branch from 6662fca to d3123dc Compare March 16, 2021 18:31

jparciga approved these changes Mar 17, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mini ch 3 #52

Mini ch 3 #52

Uh oh!

cylcrow commented Mar 8, 2021 •

edited

Loading

Uh oh!

jparciga left a comment

Uh oh!

jparciga Mar 17, 2021

Uh oh!

jparciga Mar 17, 2021

Uh oh!

jparciga Mar 17, 2021

Uh oh!

jparciga Mar 17, 2021

Uh oh!

jparciga Mar 17, 2021

Uh oh!

jparciga Mar 17, 2021

Uh oh!

jparciga Mar 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Mini ch 3 #52

Are you sure you want to change the base?

Mini ch 3 #52

Uh oh!

Conversation

cylcrow commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR do?

Any background context you want to provide?

Where should the reviewer start?

How should this be manually tested?

Screenshots

Uh oh!

jparciga left a comment

Choose a reason for hiding this comment

Mini-Challenge 3 Feedback:

Acceptance Criteria

Bonus Points

Improvements

Uh oh!

jparciga Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

jparciga Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

jparciga Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

jparciga Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

jparciga Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

jparciga Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

jparciga Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cylcrow commented Mar 8, 2021 •

edited

Loading