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

Add back 3 Stylelint configs #4350

Merged

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Nov 18, 2023

(Will wait after we're done adding things for the release)

Add back 3 Stylelint configs

Pull Request Type

  • Feature Implementation

Related issue

See chore

Description

Changes from initial (intended) config:

  • selector-no-qualifying-type now ignores attribute selectors
  • Non-recommended rule a11y/font-size-is-readable is removed (see discussion)
  • [media-prefers-reduced-motion](https://github.com/double-great/stylelint-a11y/blob/main/src/rules/media-prefers-reduced-motion/README.md) is now satisfied (but not added as a rule due to the limited capabilities of the Stylelint config)

Testing

This is a tough one to have clear testing requirements for. Many files were "changed," but pretty insubstantially so, and none should be changed functionally (excluding our new support for users with prefers-reduced-motion).

  • Test enabling prefers-reduced-motion and validate that our transitions and animations are no longer present (can be simulated by removing the media query surrounding it on line 1062 of themes.css. Time: <5 min
  • Smoke test (e.g., using settings, watching a video, and other basic features). Time: 10-20 min

Desktop

  • OS: OpenSUSE Tumbleweed
  • OS Version: 2023xxxx
  • FreeTube version: 0.19.1

Additional info

The only remaining Stylelint config of the original list to implement after this PR is stylelint-high-performance-animation. The issues to be fixed there are seemingly non-trivial and preferably placed in a separate effort.

Notably, we choose to not use the 'a11y/no-outline-none' rule. The reason for this being that our focus ring suffices for almost every use case (https://www.w3.org/WAI/WCAG22/Understanding/focus-appearance-minimum), and using the same hover and focus styling can be considered bad design in many cases. YT goes the direction of foregoing the focus ring for many of its components and choosing a barely darker color than its :hover for :focus styling, which is controversial. Accessibility-wise, at the very least, using the same hover and focus styling can be confusing, especially for sighted users who use keyboard navigation.
This is not recognized as a fix by the rule due to its limited detection logic, so this rule is not being imported directly.
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 18, 2023 18:17
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 18, 2023
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.

Too big, can't review & approve now :P

src/renderer/App.css Show resolved Hide resolved
@@ -369,6 +369,7 @@
<ft-element-list
v-if="!hideChannelCommunity && currentTab === 'community'"
id="communityPanel"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still referenced by aria-controls somewhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We still have the ID. Just added it as a class as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup I know. It's for other reviewers (coz diff won't show aria-controls

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

Copy link
Contributor

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

…ecificity

Properly implementing the no-descending-specificity rule in this file seems to utterly destroy the styling. This would be its own entire initiative to unravel.
Copy link
Contributor

github-actions bot commented Apr 6, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

@kommunarr
Copy link
Collaborator Author

kommunarr commented Apr 6, 2024

@github-actions github-actions bot added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: WIP labels Apr 13, 2024
@kommunarr
Copy link
Collaborator Author

After combing through the files and doing more testing, ft-list-channel and ft-tooltip were affected by mistakes. This is now ready for review.

PikachuEXE
PikachuEXE previously approved these changes Apr 15, 2024
@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Apr 16, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 16, 2024
PikachuEXE
PikachuEXE previously approved these changes Apr 16, 2024
@kommunarr kommunarr dismissed stale reviews from PikachuEXE and ChunkyProgrammer via 3deda96 April 16, 2024 22:43
@kommunarr
Copy link
Collaborator Author

This is currently passing for all files. Please get this one in next if possible!

@FreeTubeBot FreeTubeBot merged commit 23f1618 into FreeTubeApp:development Apr 17, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 17, 2024
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.

6 participants