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

Fix comment author focus outline being broken #4041

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

absidue
Copy link
Member

@absidue absidue commented Sep 13, 2023

Fix comment author focus outline being broken

Pull Request Type

  • Bugfix

Related issue

fixes #4025

Description

Fixes the focus outline not wrapping around the comment author, turns out the culprit was overflow: none on the .commentAuthorWrapper class. I also fixed some of the issues that stylelint was pointing out in the file.

Screenshots

before:
before

after:
after

Testing

Tab through the comments and check that the outline around the comment author names displays correctly.

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 0.19.0

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

The problem is that now the author text can overflow for sufficiently long names.

@absidue
Copy link
Member Author

absidue commented Sep 14, 2023

If we want to keep overflow set to none, we either need to add left, top and bottom margins to the channelAuthor class (the existing right margin is why the outline is visible there) or add some padding to channelAuthorWrapper, to stop the overflow setting from cropping off the outline.

@kommunarr
Copy link
Collaborator

Up to you on whether we want to keep the text-oveflow: ellipsis behavior here, but worth noting that we use word-wrap: break-word for all other text in the comments section. It works pretty well here, but I have no preference.

@absidue
Copy link
Member Author

absidue commented Sep 14, 2023

I don't really know what the best approach is to solve the problem without overflowing text.
Is your suggestion to revert the overflow: none removal and add word-wrap: break-word instead or keep the overflow: none remove and additionally add word-wrap: break-word?

@kommunarr
Copy link
Collaborator

The latter; apologies if that wasn't clear

Co-authored-by: Jason <84899178+jasonhenriquez@users.noreply.github.com>
@absidue absidue requested a review from kommunarr September 15, 2023 21:30
Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

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

LGTM

@FreeTubeBot FreeTubeBot merged commit b80d2c8 into FreeTubeApp:development Sep 18, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 18, 2023
@absidue absidue deleted the fix-comment-author-focus branch September 18, 2023 18:37
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Sep 19, 2023
* development:
  Translated using Weblate (French)
  Avoid extending the VueI18n class as it won't exist in Vue 3 (FreeTubeApp#4046)
  Fix comment author focus outline being broken (FreeTubeApp#4041)
  Bump sass from 1.66.1 to 1.67.0 (FreeTubeApp#4052)
  Bump marked from 9.0.0 to 9.0.2 (FreeTubeApp#4054)
  Bump the babel group with 2 updates (FreeTubeApp#4050)
  Bump the eslint group with 1 update (FreeTubeApp#4051)
  Bump lefthook from 1.4.10 to 1.4.11 (FreeTubeApp#4055)
  Avoid extending the VueRouter class as it won't exist in Vue 3 (FreeTubeApp#4047)
  Fix focus disappearing when visiting the settings (FreeTubeApp#4042)
  Translated using Weblate (Catalan)
  Translated using Weblate (Bulgarian)
  Translated using Weblate (Croatian)
  Translated using Weblate (Hebrew)
  Translated using Weblate (Portuguese (Brazil))
  Translated using Weblate (German)
  Translated using Weblate (Hungarian)
  Translated using Weblate (Spanish)
  Translated using Weblate (Estonian)
  Translated using Weblate (Ukrainian)
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]: Focus on comment author isnt being displayed correctly
5 participants