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

Create a Service for providing video/category data #10 #19

Merged
merged 10 commits into from
Apr 21, 2024
Merged

Create a Service for providing video/category data #10 #19

merged 10 commits into from
Apr 21, 2024

Conversation

OGTor
Copy link
Contributor

@OGTor OGTor commented Feb 19, 2024

New Feature Submissions:

This will add Services for providing video/category data

Data can be is reusable for different components.

To Test

Open project and use
npm run web
open the console in your desired web-browser
you will se different console logs with Category Labels, Videos, etc

@OGTor OGTor linked an issue Feb 20, 2024 that may be closed by this pull request
3 tasks
Copy link
Contributor

@mblomdahl mblomdahl left a comment

Choose a reason for hiding this comment

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

Please update the PR so it is to-the-point and crisp to review with respect to the scope in #10, then press the "re-request review" to invite another review when you feel it is sufficiently succinct. You should also enable the GitHub Actions workflow to demonstrate that it builds and that a reviewer can experience the new functions deployed on GitHub Pages (see PR #20 and the previous ones for good examples).

OwnTube.tv/yarn.lock Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
testData.json Outdated Show resolved Hide resolved
OwnTube.tv/package.json Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
@OGTor
Copy link
Contributor Author

OGTor commented Feb 22, 2024

Updated upon reviews

Link to commited changes

https://ogtor.github.io/web-client/

image

@OGTor OGTor requested a review from mblomdahl February 26, 2024 08:35
Copy link
Contributor

@ar9708 ar9708 left a comment

Choose a reason for hiding this comment

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

FYI, checkboxes to tick @OGTor & @tryklick :

image

TODOs:

  • Linear history
  • Signed commints

Copy link
Contributor Author

@OGTor OGTor left a comment

Choose a reason for hiding this comment

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

Final fix for testData.json.
This update should fix the problem with Actions not loading testData.json

@OGTor
Copy link
Contributor Author

OGTor commented Mar 6, 2024

Final fix for testData.json. This update should fix the problem with Actions not loading testData.json

https://ogtor.github.io/web-client/

image

mblomdahl added a commit to mblomdahl/web-client that referenced this pull request Mar 7, 2024
This PR adds test coverage for the behavior described in task OwnTube-tv#10 using Jest, which seems to be the test framework of choice for React Native (see [Testing docs here](https://reactnative.dev/docs/testing-overview#unit-tests)).

@OGTor please merge this in and update your VideoService to pass all tests without test suite modifications!
Copy link
Contributor

@mblomdahl mblomdahl left a comment

Choose a reason for hiding this comment

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

My feedback was a bit too broad to fit into the "comment on lines" format here, so I have summarized my concerns in 2 PRs from my own fork instead to provide more clear assistance. See OGTor/web-client#1 for the addition of test coverage related to #10, then see mblomdahl#1 for a small example and (primarily) a list of refactorings that are needed to make this mergeable.

@tryklick tryklick closed this Mar 12, 2024
@tryklick tryklick reopened this Mar 12, 2024
@mblomdahl mblomdahl mentioned this pull request Mar 14, 2024
9 tasks
Copy link
Contributor

@mblomdahl mblomdahl left a comment

Choose a reason for hiding this comment

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

It's looking good when I open https://ogtor.github.io/web-client/, good job! Please rebase your branch on latest main to resolve conflicts and clean up the commits (see hint in #24 (comment)). Check the boxes in on @ar9708's review #19 (review) when done. Press re-request review when you think all the file changes in this PR look correct. ⭐

mblomdahl added a commit to mkdevops-se/ogtor-web-client that referenced this pull request Mar 22, 2024
This PR adds test coverage for the behavior described in task OwnTube-tv#10 using Jest, which seems to be the test framework of choice for React Native (see [Testing docs here](https://reactnative.dev/docs/testing-overview#unit-tests)).

@OGTor please merge this in and update your VideoService to pass all tests without test suite modifications!
mblomdahl added a commit to mkdevops-se/ogtor-web-client that referenced this pull request Mar 22, 2024
This PR adds test coverage for the behavior described in task OwnTube-tv#10 using Jest, which seems to be the test framework of choice for React Native (see [Testing docs here](https://reactnative.dev/docs/testing-overview#unit-tests)).

@OGTor please merge this in and update your VideoService to pass all tests without test suite modifications!
mblomdahl added a commit to mkdevops-se/ogtor-web-client that referenced this pull request Mar 22, 2024
This PR adds test coverage for the behavior described in task OwnTube-tv#10 using Jest, which seems to be the test framework of choice for React Native (see [Testing docs here](https://reactnative.dev/docs/testing-overview#unit-tests)).

@OGTor please merge this in and update your VideoService to pass all tests without test suite modifications!
mblomdahl added a commit to mkdevops-se/ogtor-web-client that referenced this pull request Mar 22, 2024
This PR adds test coverage for the behavior described in task OwnTube-tv#10 using Jest, which seems to be the test framework of choice for React Native (see [Testing docs here](https://reactnative.dev/docs/testing-overview#unit-tests)).

@OGTor please merge this in and update your VideoService to pass all tests without test suite modifications!
mblomdahl added a commit to mkdevops-se/ogtor-web-client that referenced this pull request Mar 22, 2024
This PR adds test coverage for the behavior described in task OwnTube-tv#10 using Jest, which seems to be the test framework of choice for React Native (see [Testing docs here](https://reactnative.dev/docs/testing-overview#unit-tests)).

@OGTor please merge this in and update your VideoService to pass all tests without test suite modifications!

(Demo-rebase med @OGTor och @mblomdahl!)
Copy link
Contributor Author

@OGTor OGTor left a comment

Choose a reason for hiding this comment

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

in videoServices.test.tsx it seems as this code part (line 47 - 50)

    type Category = {
      id: number;
      label: string;
    };
    

is not used in the testing

@OGTor OGTor requested review from mblomdahl and ar9708 April 1, 2024 21:00
Copy link
Contributor

@mblomdahl mblomdahl left a comment

Choose a reason for hiding this comment

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

It's improving! Good job, here's a few comments on the actual code implementation.

Please also address the comment here, #19 (review), about signing your commits.

OwnTube.tv/Services/VideoDataService.tsx Outdated Show resolved Hide resolved
OwnTube.tv/Services/VideoDataService.tsx Outdated Show resolved Hide resolved
OwnTube.tv/Services/videoServices.tsx Outdated Show resolved Hide resolved
OwnTube.tv/Services/videoServices.tsx Outdated Show resolved Hide resolved
OwnTube.tv/Services/videoServices.tsx Outdated Show resolved Hide resolved
@OGTor OGTor self-assigned this Apr 11, 2024
Changes:
- Removed root-level package.json and package-lock.json
- Removed package.json dependency on `peertube/embed-api`
- Moved testData.json from root level to `OwnTube.tv/` Node project directory
- Load testData.json via CommonJS import instead of HTTP request to web server

From commits 4/3 to 6/3:
- github actions testData fix v11
- github actions testData fix v10
- revert
- test
- github actions testData fix v9
- github actions testData fix v8
- github actions testData fix v7
- github actions testData fix v6
- github actions testData fix v5
- github actions testData fix v4
- github actions testData fix v3
- github actions testData fix
- github actions testData fix
- Changes information in testData.json
- Fixes: Changed to using testData instead of test API
- Create a Service for providing video/category data #10

(Rebase-demo med @OGTor och @mblomdahl!)
This PR adds test coverage for the behavior described in task #10 using Jest, which seems to be the test framework of choice for React Native (see [Testing docs here](https://reactnative.dev/docs/testing-overview#unit-tests)).

@OGTor please merge this in and update your VideoService to pass all tests without test suite modifications!

(Demo-rebase med @OGTor och @mblomdahl!)
Changes:
- Updated `videoService.tsx` to pass all tests
- Updated `VideoDataService.tsx` to render app landing page with videos **in** categories

From squashed updates from 15/3 to 22/3:
- final commit v2
- Revert "Final commit", This reverts commit 5b94307.
- Final commit
- test fix v3
- test fix v2
- fix for testing
- Create a Service for providing video/category data #10
- final changes
- changed category
Copy link
Contributor

@ar9708 ar9708 left a comment

Choose a reason for hiding this comment

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

Good job! Starting to look neat and crispy. 🌟 Please see my comments below.

OwnTube.tv/components/videosOverview.tsx Outdated Show resolved Hide resolved
OwnTube.tv/types.ts Outdated Show resolved Hide resolved
OwnTube.tv/types.ts Outdated Show resolved Hide resolved
@OGTor OGTor requested a review from ar9708 April 19, 2024 06:59
Copy link
Contributor

@ar9708 ar9708 left a comment

Choose a reason for hiding this comment

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

@OGTor Your GitHub Actions pipeline is failing, please address!

@OGTor OGTor requested a review from ar9708 April 19, 2024 09:35
Copy link
Contributor

@ar9708 ar9708 left a comment

Choose a reason for hiding this comment

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

Great, looking good! 🙃

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.

Create a Service for providing video/category data
4 participants