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

Highlights comments. #783 #4245

Closed
wants to merge 0 commits into from

Conversation

ab-shrek
Copy link

@ab-shrek ab-shrek commented Oct 28, 2023

Title

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Fixes #783

Description

Highlights a comment if clicked on the comment timestamp. User can highlight unlimited comments. The page gets reloaded once clicked and the highlighted comment shows up on top of comments feed. The change does not provide any functionality to remove highlight from the comment as of now.

Screenshots

Screenshot 2023-10-28 at 7 50 01 PM Screenshot 2023-10-28 at 7 31 07 PM

Testing

Tested manually on electron app.
Clicked on comment timestamp. Noted that page gets reloaded and the highlighted comments show up on top.

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 28, 2023 13:52
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 28, 2023
@kommunarr
Copy link
Collaborator

kommunarr commented Oct 28, 2023

Hi ab-shrek, thanks for taking the initiative on this! I have a few UX criticisms, but after trying YouTube's attempt at this feature, almost all issues I have are adopted from the glaring UX problems with a multi-billion dollar company's feature (!!). So I'll mark those concerns with "YTE" for "YT-existent".

  1. (YTE) Could you add a title property to the timestamp button that highlights a comment (i.e., "Highlight Comment" / "Unhighlight Comment"? I had no idea where to even find this feature at first, let alone identify what would happen if I clicked on the comment timestamp (if not for the context of this PR).
  2. For the same reason as 1, please add some hover styling to the comment timestamp.
  3. Please add your new strings to en-US.yaml.
  4. This doesn't currently work with replies to a comment. You'll need to add this logic to the template, add a new display string ("Highlighted reply"), and ideally update the logic to bring the ancestor to the forefront as well, like done in YT below.

Screenshot_20231028_100851

  1. (YTE) Reloading the whole page on highlighting a comment seems like a pretty unpleasant UX. Could you instead do something like remove and prepend the ancestor comment (see point 4) to the commentData array?
  2. (YTE) As you mentioned, unhighlight would be great.
  3. (YTE) (non-required) Implementing unhighlight without reloading the page is tough to do accurately on subsequent page loads. But you can do this for the same non-reloaded session by storing the originalCommentIndex as a property on the comment object on highlight(), setting the original array entry to null instead of removing it as I suggested in point 5, and on unhighlight(), check to see if the comment has an originalCommentIndex first, and swap instead of reloading if so. You'd also have to update the template v-for to be v-for="(comment, index) in commentData.filter((c) => c) to preclude nulls from being rendered.

Again, great job with what's here so far; let me know if you have any questions or need any support.

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 30, 2023
auto-merge was automatically disabled November 2, 2023 19:06

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 2, 2023 19:07
auto-merge was automatically disabled November 2, 2023 19:15

Head branch was pushed to by a user without write access

@ab-shrek ab-shrek closed this Nov 2, 2023
auto-merge was automatically disabled November 2, 2023 19:15

Pull request was closed

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

Hi @ab-shrek why did u close this PR and create a new one?

@ab-shrek
Copy link
Author

ab-shrek commented Nov 2, 2023

I am sorry; got into a weird mess and tried undoing it which led to this PR being closed. Can you please check the latest PR instead?

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.

Highlighting comments
4 participants