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

Mini ch 4 #58

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

Mini ch 4 #58

wants to merge 27 commits into from

Conversation

cylcrow
Copy link

@cylcrow cylcrow commented Mar 18, 2021

What this PR do?

Performs tasks for mini challenge 4

Any background context you want to provide?

Big refactors, added lots of new tests. 👀 🧪

Where should the reviewer start?

How should this be manually tested?

Screenshots

image
image
image
image

OMG! 😃

cylcrow and others added 26 commits February 19, 2021 16:16
- Removed unnecessary files
- Added first tests
- Added first components
- Added dependencies for testing
- 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
- Added AppContext to share global context
- Moved theme spec to AppContext
- Moved useSearch to App.jsx component
- Added appReducer for global context: AppContext
- Deleted unnecessary files
- Added test for useReducer, googleMockedAPIObject files
- Prevented props passing from parent componenst to child componenst using context props
- Reformated code on some files
Copy link

@jparciga jparciga left a comment

Choose a reason for hiding this comment

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

Mini-Challenge 4 Feedback

Great job 👏🏻!

Acceptance Criteria

✅ The search term is stored and retrieved from the Global Context correctly. - Not exactly, but the desired functionality was achieved.
✅ The appearance theme is stored on the Global Context and applied correctly to the App UI.
✅ useReducer hook is implemented correctly to manage the Global State.

Bonus Points

✅. Testing coverage is above 70%. (Please include a screenshot of the code coverage report in your PR description).

Screen Shot 2021-03-18 at 15 59 38

Comment on lines +46 to +54
it('applies background for light theme', async () => {
const { firstChild } = (await build()).container;
expect(firstChild).toHaveStyle(`background: ${lightTheme.color.background}`);
});

it('applies background for dark theme', async () => {
const { firstChild } = (await build(<LayoutWrapper/>, darkTheme)).container;
expect(firstChild).toHaveStyle(`background: ${darkTheme.color.background}`);
});

Choose a reason for hiding this comment

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

These are good test cases 👏🏻

Copy link
Author

Choose a reason for hiding this comment

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

OMG, I wasn't expecting a review so soon.
I forgot to add a screenshot of the test coverage, I also added an update.
This is the new coverage:
image
Given that the gapi object is available once the youtube script is loaded, I wasn't being checking that behavior.

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