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

Refactoring to community polls & fixes for quizzes #3865

Merged
merged 12 commits into from
Aug 24, 2023

Conversation

lamemakes
Copy link
Contributor

Refactoring to community polls & fixes for quizzes

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Closes #3746.

Description

This PR implements community quizzes for both the local and invidious API, giving users an interactive option to take the quiz that requires no callbacks to YouTube. This also fixes the vote count format changing between local/indivious APIs

Screenshots

Before:

Broken quiz prompt:
Screenshot from 2023-08-09 08-57-42

Poll vote count on local API:
Screenshot from 2023-08-09 08-58-49

Poll vote count on Invidious API:
Screenshot from 2023-08-09 08-57-56

After:

Quiz prompt with options:
Screenshot from 2023-08-09 08-09-25

Selecting the correct answer:
Screenshot from 2023-08-09 08-12-28

Selecting the incorrect answer:
Screenshot from 2023-08-09 08-12-40

New vote count that is API independent:
Screenshot from 2023-08-09 08-10-11

Testing

Channel communities this was tested on:

These worked good for testing the vote count side of things, but Lofi Girl was the only one that really allowed me to test quizzes. If anyone else knows where to find more YT community quizzes, please feel free to comment them.

Desktop

  • OS: Fedora
  • OS Version: 37
  • FreeTube version: aa1d422

Additional context

A new component was created that is intended to be used for both polls and quizzes due to their similarity, so this avoids duplicate code.

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) August 9, 2023 13:43
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Aug 9, 2023
Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Haven't tested it yet but have reviewed the code and left a bunch of comments.

As for the quiz implementation, I would prefer to have a "reveal correct answer" button instead of making the quiz interactive, as the interactivity of the quiz gives the user the impression that their answer is being transmitted to YouTube.

@lamemakes
Copy link
Contributor Author

These comments are super helpful! I'll patch them up soon and have it ready for you. I'm definitely a fan of the reveal button, that makes a lot more sense and will probably simplify the code quite a bit.

auto-merge was automatically disabled August 11, 2023 16:20

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) August 11, 2023 16:21
auto-merge was automatically disabled August 11, 2023 16:33

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) August 11, 2023 16:33
@lamemakes
Copy link
Contributor Author

The latest layout can be seen here:
Screenshot from 2023-08-11 12-34-04

when reveal answers is selected:
Screenshot from 2023-08-11 12-34-09

when hide answers is selected:
Screenshot from 2023-08-11 12-34-18

Is there a theme color/addition you wanted me to make for the green? Right now that's hard coded but I could add/pull something from the master theming file if that would be beneficial.

auto-merge was automatically disabled August 11, 2023 16:47

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) August 11, 2023 16:47
@lamemakes
Copy link
Contributor Author

@absidue Thanks for bearing with me here! This was super educational on FreeTube's flow and practices. Let me know if you have any other suggestions.

@lamemakes lamemakes requested a review from absidue August 11, 2023 16:49
@absidue
Copy link
Member

absidue commented Aug 11, 2023

My only concern at the moment, is that the reveal/hide answer button isn't accessible, however that shouldn't be too hard to fix. Here is how it is done for the Fetch more results button on the search page, you'll also want to update the hover CSS to also get activated for the focus pseudo class:

<div
class="getNextPage"
role="button"
tabindex="0"
@click="nextPage"
@keydown.enter.prevent="nextPage"
@keydown.space.prevent="nextPage"
>
<font-awesome-icon :icon="['fas', 'search']" /> {{ $t("Search Filters.Fetch more results") }}
</div>

@absidue
Copy link
Member

absidue commented Aug 11, 2023

Apart from that accessibility issue the code looks good :). I haven't tested it yet, but I'll get onto that soon.

auto-merge was automatically disabled August 11, 2023 17:23

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) August 11, 2023 17:23
Copy link
Member

Choose a reason for hiding this comment

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

Local API doesnt seem to work when i try to reveal the answers

IV-API.mp4
Local-API.mp4

@lamemakes
Copy link
Contributor Author

Interesting - I couldn't recreate this. My development happened using the local API so I'll poke around and see if anything is different in my env

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Aug 11, 2023

It could be that my env is buggy. If others aren't experiencing it then I'm happy to approve

@PikachuEXE
Copy link
Collaborator

I can reproduce the reveal answer issue for local API (IV is fine)
I tried @efb4f5ff-1298-471a-8973-3d47447115dc posted one and the following:
image

@absidue absidue mentioned this pull request Aug 15, 2023
1 task
auto-merge was automatically disabled August 17, 2023 17:28

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) August 17, 2023 17:28
@lamemakes
Copy link
Contributor Author

Great catch! I can't figure out why it wouldn't break locally, but it should be fixed now. Been on travel this week so I apologize for the delay.

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Inconsistent indent
Ensure your editor supports .editorconfig - https://editorconfig.org

Copy link
Member

Choose a reason for hiding this comment

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

LGTM now just waiting on the requested changes to be implemented

auto-merge was automatically disabled August 21, 2023 12:27

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) August 21, 2023 12:27
@PikachuEXE
Copy link
Collaborator

@absidue

@FreeTubeBot FreeTubeBot merged commit 4a7c4d9 into FreeTubeApp:development Aug 24, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Aug 24, 2023
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.

[Bug]: Broken Community post
6 participants