-
-
Notifications
You must be signed in to change notification settings - Fork 254
Fix Role class path for ::getNavigationIcon()
#1936
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
Conversation
📝 WalkthroughWalkthroughCentralizes navigation icon resolution: Changes
Pre-merge checks❌ Failed checks (2 warnings, 1 inconclusive)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/Models/Role.php (2)
88-94: Custom icon registry matches existing extension patternThe
static $customModelIconsstore plusregisterCustomModelIcon()mirror the existingregisterCustomPermissions()approach and correctly allow overrides on a per‑model basis. The overwrite semantics on repeat registration are reasonable; only optional improvement would be to add a way to clear this registry in tests if you ever need isolation between test cases.
141-164: Icon resolution order looks good; consider name normalization & confirm null contractThe resolution order (custom → defaults → Admin resource → Admin page → Server resource →
null) matches the intended behavior and the new panel‑specific namespaces look consistent with howRoleResourceis namespaced.Two small follow‑ups to consider:
- Using
ucwords($model)plus a naïve's'suffix can be brittle if any model keys ever contain underscores/dashes or are already pluralized (e.g."activity_log","settings"). Using Laravel’sStr::studly($model)would be more robust if you expect such keys.getModelIcon()explicitly returns?string; ensure all current call sites (e.g. section->icon()usage) acceptnulland behave as expected when no icon is resolved. If you always want an icon, you might add a final fallback (e.g.'tabler-help-circle').
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Filament/Admin/Resources/Roles/RoleResource.php(1 hunks)app/Models/Role.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/Models/Role.php (1)
app/Filament/Admin/Pages/Health.php (2)
icon(163-172)getNavigationIcon(129-139)
app/Filament/Admin/Resources/Roles/RoleResource.php (1)
app/Models/Role.php (2)
Role(24-183)getModelIcon(141-164)
🔇 Additional comments (2)
app/Models/Role.php (1)
57-61: Static MODEL_ICONS mapping is clear and consistentThe built‑in mapping for
health,activityLog, andpanelLogaligns with the permission keys and gives you stable icons regardless of how the underlying Filament page/resource chooses its navigation icon. No issues here.app/Filament/Admin/Resources/Roles/RoleResource.php (1)
174-174: Nice centralization of icon resolutionWiring
Section::icon()throughRole::getModelIcon($model)removes the ad‑hoc class resolution from this resource and keeps icon logic in one place. AssumingSection::icon()toleratesnull(for models without an icon), this looks solid; otherwise you may want a fallback icon there.
Filament v4 changed the folder architecture.