-
-
Notifications
You must be signed in to change notification settings - Fork 28
Supporting group title, icon and order #77
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
base: main
Are you sure you want to change the base?
Conversation
7ec391b
to
6727698
Compare
Hi, please give me a heads up when the PR is ready to be reviewed and merged. I see you still do some changes to it. I converted it to a draft PR for the meantime. If it already is ready, just mark it as ready to review again. |
It's ready for review/feedback/discussion. I was just adding some bug fixes I ran into when using it in one of my projects. |
Hello Leo, thanks again for the PR and sorry for taking so long! Didn't have time any sooner. So I tested it out locally and everything seems to be working as expected, well done! Are you able to document the usage of the |
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.
@lukas-frey, I would like your feedback on this change.
This is necessary to make this PR work for 3-level, but also means that the 2-level using parent frontmatter doesn't work without setting an icon manually on the parent.
I'm leaning towards keeping this change, but adding smart detection to the nav building like so:
- level 1 w/o children get icon if not set
- level 1 w/ children 1 deep get icon if not set
- level 2 get icon if not set (it's ignored if parent has icon)
I think that will preserve the current behavior.
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.
Hi, can you please elaborate on the nav building detection.
Basically prior to your PR, we always set an icon on every navigation item (heroicon-o-document
), which you need to remove in order to support navigation items that have no icon. But now when nesting child items under a parent item, this will throw an exception when the parent has no icon.
As far as I understand, this is the only place where not having an icon will cause an issue. So if there is a navigation item with child items which has not an icon set, we need to set some default.
Do I understand it correctly that this is this your suggestion?
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.
I think you've got it right.
I guess the basic difference is adding it during the navigation building.
This will also make it "driver" agnostic. On main, the default icon is being added in the flat file model.
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.
Yeah, there's currently no other working driver anyways, although there should have been a working database "driver". But I have many features on the roadmap that will probably be added to v2 as part of a bigger rewrite anyways.
Anyways, I think your suggestion makes sense, feel free to add it to the PR. Thanks a lot for the time you're investing in the package!
I haven't forgotten this PR. Just haven't had time yet to work on the docs and finish the icon change. |
No worries! I'll mark it as draft for now. When you are ready, you can mark it for review again. :) |
This will probably need several iterations to get right, but here's what I've got working.
Features
_group.md
supports title:, icon: and order:_group.md
will show, regardless if usedDepth options
Note: FilamentPHP only allows icons at one level
1] Document with icon
2] Document with icon and sub-documents using parent:
3] Group with icon and sub-documents
4] Group without icon and documents with icons