Skip to content

Don't request category default filters on pages that don't have categories#1827

Merged
Oaphi merged 10 commits intodevelopfrom
0valt/1808/unnecessary-ajax
Sep 14, 2025
Merged

Don't request category default filters on pages that don't have categories#1827
Oaphi merged 10 commits intodevelopfrom
0valt/1808/unnecessary-ajax

Conversation

@Oaphi
Copy link
Copy Markdown
Member

@Oaphi Oaphi commented Sep 12, 2025

Partially addresses #1808 (unnecessary requests for category default filters on pages that don't have a category such as user tabs). As a side-effect, finally adds proper Prettier config to format our JS assets (note that prettier-plugin-brace-style is needed to support our choice of brace style - Prettier's team is dead set on not supporting it).

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.87%. Comparing base (e2a295b) to head (edb7bd2).
⚠️ Report is 15 commits behind head on develop.

Additional details and impacted files
Components Coverage Δ
controllers 71.66% <100.00%> (+0.13%) ⬆️
helpers 82.01% <ø> (ø)
jobs 60.00% <ø> (ø)
models 89.02% <ø> (ø)
tasks 61.11% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Member

@cellio cellio left a comment

Choose a reason for hiding this comment

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

I tested to make sure all affected pages load fine and they do. There are other pages that don't have categories (search results and help are the ones that immediately came to mind); I assume that we can make a similar change there later but don't need to do anything now? (The pages look fine in my testing.)

I started to review the JS but quickly got in over my head. Left a couple markers in places where I'd figured out that an apparent diff isn't (to help future reviewers), then bailed on that part. We'll need someone more fluent there.

@trichoplax
Copy link
Copy Markdown
Contributor

trichoplax commented Sep 12, 2025

Prettier's team is dead set on not supporting it

Is the brace style that Prettier doesn't support one that our JS-facing team prefers, or has just inherited? Would there be any objection to adopting Prettier's style?

Is the decision affected by whether Prettier will autoformat so we don't have to make any manual changes?

@Oaphi
Copy link
Copy Markdown
Member Author

Oaphi commented Sep 13, 2025

Is the brace style that Prettier doesn't support one that our JS-facing team prefers, or has just inherited? Would there be any objection to adopting Prettier's style?

@trichoplax Stroustrup's brace style is in our style guidelines and, I believe, @ArtOfCode- prefers it too. I personally don't have an opinion on the matter (I rarely do when it comes to formatting - happy to work with any as long as a formatter can be configured to automatically bring code into required shape).

The other brace style (same line) is much more common, though, for JS, and will save us from having to add a plugin, but it's not my decision to make (we might also want to try an alternative formatter - I just haven't used any except for Prettier for years at this point, it's extremely popular and is quite polished, even if very opinionated). Deferring to @ArtOfCode-

@trichoplax
Copy link
Copy Markdown
Contributor

I personally don't have an opinion on the matter (I rarely do when it comes to formatting - happy to work with any as long as a formatter can be configured to automatically bring code into required shape).

I feel the same way. Only asked in case it would save us a dependency (if everyone happens to not mind).

@Oaphi Oaphi merged commit 7c3c26e into develop Sep 14, 2025
13 checks passed
@Oaphi Oaphi deleted the 0valt/1808/unnecessary-ajax branch September 14, 2025 03:41
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.

4 participants