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 FilterMenu not saving simple filters #4836

Merged
merged 3 commits into from
Oct 12, 2023

Conversation

dmitrylyzo
Copy link
Contributor

@dmitrylyzo dmitrylyzo commented Oct 4, 2023

Regression 6341a71

Changes

  • Move "simpleFilter" query to where it is used.
  • Move queries into loops.

Issues
FilterMenu doesn't save simple filters because of elems override.

@dmitrylyzo dmitrylyzo added bug Something isn't working regression We broke something labels Oct 4, 2023
@dmitrylyzo dmitrylyzo requested a review from a team as a code owner October 4, 2023 10:19
for (let i = 0, length = elems.length; i < length; i++) {
if (elems[i].checked) {
videoTypes.push(elems[i].getAttribute('data-filter'));
for (const elem of context.querySelectorAll('.chkVideoTypeFilter')) {
Copy link
Contributor Author

@dmitrylyzo dmitrylyzo Oct 4, 2023

Choose a reason for hiding this comment

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

forEach is more portable (for legacy) than for..of. 🤔

for..of is transpiled into a large block with iterator.
☝️ this bothers me.

Copy link
Member

Choose a reason for hiding this comment

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

It sounds like we should setup some eslint rules to prevent us from using some of these language features that make generated code more complex since it has came up a few times now. I can also disable related rules on the sonar side as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried eslint-plugin-no-for-of-loops: 147 errors 😅

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to change the usages here or leave it as is?

@sonarcloud
Copy link

sonarcloud bot commented Oct 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.1% 1.1% Duplication

@jellyfin-bot
Copy link
Collaborator

Cloudflare Pages deployment

Latest commit c941a44
Status ✅ Deployed!
Preview URL https://3605d5b3.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs
View bot logs

@thornbill thornbill added this to the v10.9.0 milestone Oct 12, 2023
@thornbill thornbill merged commit fee6568 into jellyfin:master Oct 12, 2023
22 checks passed
@dmitrylyzo dmitrylyzo deleted the fix-filtermenu branch October 12, 2023 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working regression We broke something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants