-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Reimagine Navigation rendering features #199
base: 3.x
Are you sure you want to change the base?
Reimagine Navigation rendering features #199
Conversation
This PR concept starts to add ability to register renderers like custom Nav item types. It also implements first-party examples of using the new system with the built-in nav item types.
d8e583f
to
42c3cb3
Compare
I won't be mad if you feel that overall this isn't worth adding to the core project to maintain. So just feel free to LMK and close it if that's the case. Otherwise I'm going to leave a review of things I'm wondering about and would like feedback on. |
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.
Left my general comments; some are just general explanation to remind me when I come back to this PR and others are seeking feedback/ideas.
} | ||
|
||
// TODO: something to control these classes for end-user? | ||
public static string $activeClasses = 'border-b border-b-secondary-500 text-secondary-500'; |
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.
As mentioned - having something to control this could be nice, tho not necessary immediately.
|
||
abstract public function getModel(): ?Model; | ||
|
||
abstract public function getLink(): ?string; |
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.
Think this one was made nullable on accident 🤔 - technically the most "empty" thing we need out of this is an empty string. Don't think it actually needs to be able to be null?
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.
a use case come to mind, I did use it in on project but for the love of god cant find which one was...
the menu can have nested items, for a dropdown menu with sub nav, so maybe in this case the link can be null!
thank you @mallardduck so much, amazing work, I love it. thank you again. |
appreciate the collaboration here @atmonshi ! no rush at all for a review - I've been working on this during my lunch RN. but plan to come back to this in a few hours after work. so I may take a second pass at this before it needs a through review. (maybe I can find an elegant solution for optional span wrappers). I can also start poking around the rest of your eco-system to see how those overlap and play with these features too! |
thank you. |
I think that's OK! I actually started building this MVP based on my local project. namespace App\VendorOverrides\Sky;
use LaraZeus\Sky\Classes\RenderNavItem;
class RenderNavItemLink extends RenderNavItem
{
public static function render(array $item, string $class = ''): string
{
if ($item['type' ] === 'local-route' || $item['type'] === 'local_route') {
return '<a class="' . $class . '"
target="' . ($item['data']['target'] ?? '_self') . '"
href="' . route($item['data']['route']) . '"
>' .
$item['label'] .
'</a>';
}
return parent::render($item, $class);
}
} And I can still even use this method to render the nav. So I don't think this should introduce any breaking changes for apps already using custom |
@atmonshi - I think this is a more user friendly way to do it now. Being able to publish the view makes it super easy to customize. And anyone needing more than this can copy the component fully and render their project local version. Let me know when you have time to review and give feedback. Once we land on stable API I can work on docs. Edit: Actually just realized i still have a TODO of "figure out how to let users configure |
src/Configuration.php
Outdated
return $this; | ||
} | ||
|
||
public function getNavRenderers(): array |
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.
ok, first note: we cant use the plugin configuration class for any FE stuff. since the panel is not available, and Filament
will load the default one. and in some cases the plugin is not registered in the default panel.
so if I have AdminPanel
and CmsPanel
and sky only registered in CmsPanel
but the ->default()
is CmsPanel
... :) will it will throw an exception.
that is way I kept the config file, for all frontend stugg. just need to move this one to the config file.
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.
ohh - fair call out. I hadn't gotten to my task of "customize the zeus theme/template" yet! I've only been testing the navigation in my own sites templates so far. So I think once I setup the Blog's theme to match my template it'll make more sense to me what's going on here.
|
||
abstract public function getModel(): ?Model; | ||
|
||
abstract public function getLink(): ?string; |
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.
a use case come to mind, I did use it in on project but for the love of god cant find which one was...
the menu can have nested items, for a dropdown menu with sub nav, so maybe in this case the link can be null!
the component is 👌🔥 |
Motivation
As I was building my site using Sky/filament I was considering ways this DX can be improved. I really liked how easy it was to add in my own item type. After fixing bug #196 things we working very well with my route based links. However I wanted to have equally as simple and clean DX when rendering the anchor link too.
So I've begun working on a similar syntax for registering custom Nav link renderers too!
This PR concept starts to add ability to register renderers like custom Nav item types.
It also implements first-party examples of using the new system with the built-in nav item types.
New Feature Usage Example:
In my case, I'm using a "Local Route" custom item type; code here:
Registering the new link type's renderer via
zeus-sky.php
config:Customize the default classes for active/non-active links:
Within the AppServiceProvider class.
Further customizing the Anchor link:
If someone wants to add a label wrapper around the link text this is easy now.
They simply publish the zeus views - then can delete all of them but
views/vendor/zeus/components/sky-link.blade.php
, then edit it according to their needs. In my example, I would use:With the additional SPAN element which the default system would not include.
TODO: