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

Added Button to Show Original Title and Thumbnail for DeArrow #6164

Open
wants to merge 16 commits into
base: development
Choose a base branch
from

Conversation

JL0000
Copy link

@JL0000 JL0000 commented Nov 14, 2024

Added Button to Show Original Title and Thumbnail for DeArrow

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #3900

Description

If DeArrow is enabled for title or/and thumbnail, when hovering over the video info area, a button is added next to the video title when the video is part of a list. This button switches the title or/and thumbnails back to its original version and vice versa.

  • The smaller button suggests that the title content did not change (case insensitive).

Video

after.mov

Testing

  • Button doesn't show up when DeArrow is not enabled
  • The button cannot be pinned if DeArrow did not change anything.

Desktop

  • OS: MacOS
  • OS Version: 15.1
  • FreeTube version: v0.22.0 Beta

Additional context

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 14, 2024 00:12
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 14, 2024
@PikachuEXE
Copy link
Collaborator

Even though it feels kind of working for desktop
Reducing padding for buttons is bad for mobile users (touch screen users more precisely)
Functionality works but UI needs to be improved but I have no idea what would be good

@absidue
Copy link
Member

absidue commented Nov 14, 2024

I agree with @PikachuEXE here, maybe it would be better suited inside the dropdown?

@JL0000
Copy link
Author

JL0000 commented Nov 14, 2024

Didn't know it was available on mobile. What's the best way to test on mobile?

@JL0000
Copy link
Author

JL0000 commented Nov 14, 2024

@absidue I think putting it in the dropdown menu would make it too cumbersome to use this feature.

@absidue
Copy link
Member

absidue commented Nov 14, 2024

If you don't want to move it to the dropdown where the rest of the options are then you'll have to stick with your current solution but undo the change to shrink the buttons.

@efb4f5ff-1298-471a-8973-3d47447115dc

I think that we shouldn't put this in the dropdown and keep this as is.

Some differences im seeing compared to #3900 (comment)

title moves around on hover

VirtualBoxVM_uhd6mYOPQa.mp4

button has different sizes

VirtualBoxVM_48KHtSzYNe.mp4

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 15, 2024
auto-merge was automatically disabled November 15, 2024 11:22

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 15, 2024 11:22
@JL0000
Copy link
Author

JL0000 commented Nov 15, 2024

@efb4f5ff-1298-471a-8973-3d47447115dc I see the title also moving on hover in #3900 (comment)? About the button size, in the DeArrow extension there are two different button sizes.

brave.mov

@JL0000
Copy link
Author

JL0000 commented Nov 15, 2024

I have changed back the button padding to its original size.

Thinking of adding a new setting in the DeArrow setting to enable this toggle to see original title/thumbnail.

@efb4f5ff-1298-471a-8973-3d47447115dc

I think https://fontawesome.com/icons/circle-dot?f=classic&s=solid is a better representation of the DeArrow button that is used on the YT side.

Dot enabled color should be based on the secondary color theme
Dot disabled should be grey

firefox_s0pK2XtSBu.mp4

Title moves around on hover

VirtualBoxVM_GJQZKbc0BW.mp4

Doenst hover here

firefox_w2XRWfIjM2.mp4

auto-merge was automatically disabled November 22, 2024 16:35

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 22, 2024 16:35
@JL0000
Copy link
Author

JL0000 commented Nov 22, 2024

  • The dot is now enabled if either the title or thumbnail has been modified. It will be in the primary colour.
  • If not enabled, it is smaller and in grey
  • The title hovering has now been fixed.
Screen.Recording.2024-11-22.at.16.40.17.mov
  • The toggle feature is in the settings. It is off by default.
Screenshot 2024-11-22 at 16 33 00

auto-merge was automatically disabled November 22, 2024 16:47

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 22, 2024 16:48
class="deArrowToggleButton"
:class="{alwaysVisible: deArrowTogglePinned}"
:icon="['fas', 'dot-circle']"
:color="deArrowChangedContent ? 'var(--primary-color)' : '#808080'"
Copy link
Member

@absidue absidue Nov 23, 2024

Choose a reason for hiding this comment

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

Please use the existing theme variables, don't hardcode colours, as it has to be usable with all app-wide themes. Additionally the ft-icon-button component already has a theme property for changing the colours of it, please use that instead of introducing another way to do the same thing.

Copy link
Author

Choose a reason for hiding this comment

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

I tried using the primary theme in ft-icon-button but the colours are reversed. I don't think I should create a new theme or change the current one. Not sure how to proceed.

ft-icon-button.scss

&.primary {
    background-color: var(--primary-color);
    color: var(--text-with-main-color);

Current
image
If I reverse color with background-color
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

One fix would be to use the --favorite-icon-color as the "enabled" mode color, as that's one we already have set as a universal "other" state. You could also do filter: grayscale(1); for the disabled state and the primary/secondary icon color for the enabled state, which would match better with the existing DeArrow web UI.

Copy link
Author

Choose a reason for hiding this comment

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

In the end I just ended up using <font-awesome-icon> and creating a custom css class for the button because the main and hover colours are different.

Comment on lines 52 to 54
deArrowShowOriginalAlwaysOn: function () {
return this.$store.getters.getDeArrowShowOriginalAlwaysOn
},
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this value doesn't exists in the store and is also not used in the component. This is presumably a leftover from previous iterations of this pull request?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah that is leftover, my bad

@efb4f5ff-1298-471a-8973-3d47447115dc
  • I would like the Show "Show Original" Button to be greyed out when the DeArrow toggles are disabled

  • Doesnt change to grey for me

VirtualBoxVM_LU41sPu3t4.mp4

@kommunarr
Copy link
Collaborator

question: Why is there a "Show Original" toggle that's disabled by default? Is this degree of optionality important?

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 23, 2024 22:37
auto-merge was automatically disabled November 23, 2024 23:33

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 23, 2024 23:33
auto-merge was automatically disabled November 25, 2024 14:40

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 25, 2024 14:40
auto-merge was automatically disabled November 25, 2024 20:37

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 25, 2024 20:37
@ChunkyProgrammer ChunkyProgrammer added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Nov 25, 2024
Copy link
Member

Choose a reason for hiding this comment

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

LGTM! Really nicely put together I hope we will see more wonderful contributions from you :)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for this :(

Noticed this one when spamming the DeArrow icon. Characters from the video next to it will highlight when spamming the icon.

VirtualBoxVM_FEra9TLJWk.mp4

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 25, 2024
@PikachuEXE
Copy link
Collaborator

@efb4f5ff-1298-471a-8973-3d47447115dc
That's just same behaviour on other text (and other non button places)
Unless it doesn't happen in dev

Screen.Recording.2024-11-26.at.06.47.11.mp4

@efb4f5ff-1298-471a-8973-3d47447115dc

Hmm im a bit confused as it doesnt happen here

VirtualBoxVM_cTienpH8RG.mp4

@PikachuEXE
Copy link
Collaborator

I confirm mouse clicks would select nearby test for some reason in dev
Not this PR specific

@JL0000
Copy link
Author

JL0000 commented Nov 26, 2024

I think this happens because of v-if and if you click on the button too quickly, it clicks on the background which selects the text?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Reveal original title when using DeArrow
6 participants