Removed sources filter link from stats page#22683
Conversation
no ref Sources filtering is inconsistent so we've disabled the filtering functionality until we can fix it.
WalkthroughThe pull request disables the navigation functionality within the TopSources component by commenting out the call to the Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ghost/admin/app/styles/layouts/stats.cssOops! Something went wrong! :( ESLint: 8.44.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
ghost/admin/app/components/stats/charts/top-sources.jsOops! Something went wrong! :( ESLint: 8.44.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ghost/admin/app/styles/layouts/stats.css (1)
305-311: Consider using CSS inheritance instead of duplicating properties.The new
.gh-stats-bar-text-nolinkclass duplicates all properties from.gh-stats-bar-textexcept for the hover behavior. Consider using CSS inheritance to reduce duplication and improve maintainability:-/* Existing class */ .gh-stats-bar-text { display: flex; align-items: center; gap: 6px; color: var(--black); line-height: 1.3em; } -/* New duplicated class */ -.gh-stats-bar-text-nolink { - display: flex; - align-items: center; - gap: 6px; - color: var(--black); - line-height: 1.3em; -} +/* Shared base class */ +.gh-stats-bar-text-base { + display: flex; + align-items: center; + gap: 6px; + color: var(--black); + line-height: 1.3em; +} + +/* Interactive version */ +.gh-stats-bar-text { + composes: gh-stats-bar-text-base; + cursor: pointer; +} + +/* Non-interactive version */ +.gh-stats-bar-text-nolink { + composes: gh-stats-bar-text-base; + cursor: default; +}This approach would eliminate the need for the inline style in the component.
ghost/admin/app/components/stats/charts/top-sources.js (2)
84-89: Consider a cleaner approach to disable filtering functionality.The current implementation appears to be a temporary solution to disable the filtering functionality. Consider these improvements:
- The cursor style is defined inline while it could be part of the CSS class
- The navigation function call is commented out rather than conditionally applied
For a more maintainable solution:
<a href="#" - style={{cursor: 'default'}} onClick={(e) => { e.preventDefault(); - // this.navigateToFilter(label || 'direct'); + if (this.args.enableSourceFiltering) { + this.navigateToFilter(label || 'direct'); + } }} className="gh-stats-bar-text-nolink" >This approach would make it easier to re-enable the functionality in the future without having to uncomment code.
42-52: Functionality temporarily disabled but method still exists.The
navigateToFiltermethod andupdateQueryParamsmethod are now unused since the click handler has been commented out. Since this appears to be a temporary change until a proper fix is implemented, it's appropriate to keep these methods. However, consider adding a TODO comment to indicate this is a temporary solution:@action navigateToFilter(source) { + // TODO: Source filtering temporarily disabled due to inconsistency issues + // Will be re-enabled after implementing a proper fix (see PR #22683) this.updateQueryParams({source}); }This helps other developers understand why this functionality exists but isn't currently being used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/app/components/stats/charts/top-sources.js(1 hunks)ghost/admin/app/styles/layouts/stats.css(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Admin tests - Chrome
1 participant
no ref
Sources filtering is inconsistent so we've disabled the filtering functionality until we can fix it.