Reimagine Navigation rendering features #199

mallardduck · 2024-03-14T18:09:50Z

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:

  ->itemType(
      name: "Local Route",
      fields: function(): array {
          return [
              Select::make('route')
                  ->label("Route")
                  ->options([
                      '' => "Select a Route",
                      ...RouteListOptions::getOptions(), // Not a complete example, this is only in my project
                  ])
                  ->default('')
                  ->selectablePlaceholder(false),
              Select::make('target')
                  ->label("Target")
                  ->options([
                      '' => "Current",
                      '_blank' => "New Tab",
                  ])
                  ->default('')
                  ->selectablePlaceholder(false),
          ];
      },
      slug: 'local-route'
  )

Registering the new link type's renderer via zeus-sky.php config:

    'navRenderers' => [
        \App\VendorOverrides\Sky\LocalRouteRenderer::class,
    ],

class LocalRouteRenderer extends NavLinkRenderer
{
    public static string $rendersKey = 'local-route';

    public function getModel(): ?Model
    {
        return null;
    }

    public function getLink(): string
    {
        return route($this->item['data']['route']);
    }

    public function isActiveRoute(): bool
    {
        return false;
    }
}

Customize the default classes for active/non-active links:

  public function register()
  {
      RenderNavItem::setActiveClasses("active border-b border-b-pink-300");
      RenderNavItem::setNonActiveClasses('');
  }

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:

<a {{ $attributes->except('label')->except('hasLabelWrap') }}>
    <span class="link-text">{{ $label }}</span>
</a>

With the additional SPAN element which the default system would not include.

TODO:

Land on a stable API we like
Go back and document the stable API

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.

mallardduck · 2024-03-14T18:16:46Z

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.

mallardduck

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.

src/Classes/LinkRenderers/GenericLinkRenderer.php

src/Classes/LinkRenderers/LibraryLinkRenderer.php

mallardduck · 2024-03-14T18:20:46Z

src/Classes/LinkRenderers/NavLinkRenderer.php

+    }
+
+    // TODO: something to control these classes for end-user?
+    public static string $activeClasses = 'border-b border-b-secondary-500 text-secondary-500';


As mentioned - having something to control this could be nice, tho not necessary immediately.

mallardduck · 2024-03-14T18:21:49Z

src/Classes/LinkRenderers/NavLinkRenderer.php

+
+    abstract public function getModel(): ?Model;
+
+    abstract public function getLink(): ?string;


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?

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!

src/Classes/RenderNavItem.php

src/Classes/LinkRenderers/NavLinkRenderer.php

atmonshi · 2024-03-14T18:31:50Z

thank you @mallardduck so much, amazing work, I love it.
initially, I will accept it.
but give me a few days and I'll back to this and will response to all feedback, and test it with the integration for other packages (bolt, thunder, etc) too.

thank you again.

mallardduck · 2024-03-14T18:34:53Z

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!

atmonshi · 2024-03-14T18:37:35Z

thank you.
so I didnt check everything yet. but will this consider a breaking change!
if an a app already using custom RenderNavItem?

mallardduck · 2024-03-14T18:43:44Z

but will this consider a breaking change!
if an a app already using custom RenderNavItem?

I think that's OK! I actually started building this MVP based on my local project.
And I was using a custom RenderNavItem that called parent::render($item, $classes); for anything I wasn't modifying.
Like:

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 RenderNavItem - assuming doing it like I show above was the "best-practice". Since while the code is very different looking internally, it does still produce identical results in terms of output. 😄

mallardduck · 2024-03-18T20:53:50Z

@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 activeClasses and defaultActiveClass. So that should be sorted out - and I think potentially the names of those might be a little off. defaultActiveClass maybe should be defaultNonActiveClass but I named it wrong?

atmonshi · 2024-03-18T21:12:08Z

src/Configuration.php

+        return $this;
+    }
+
+    public function getNavRenderers(): array


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.

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.

src/Classes/LinkRenderers/GenericLinkRenderer.php

src/Classes/LinkRenderers/NavLinkRenderer.php

atmonshi · 2024-03-18T21:16:12Z

src/Classes/LinkRenderers/NavLinkRenderer.php

+
+    abstract public function getModel(): ?Model;
+
+    abstract public function getLink(): ?string;


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!

atmonshi · 2024-03-18T21:17:20Z

the component is 👌🔥

mallardduck and others added 5 commits March 14, 2024 14:03

WIP: reimagine Navigation rendering features

8ba78d8

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.

Fix styling

bbeda2e

Add more notes for todo tasks

cd0526f

remove old TODO i already addressed

af8a186

remove non-working idea from code for now

42c3cb3

mallardduck force-pushed the configurable-nav-renderers branch from d8e583f to 42c3cb3 Compare March 14, 2024 18:15

mallardduck commented Mar 14, 2024

View reviewed changes

mallardduck and others added 2 commits March 18, 2024 16:43

refactor: introduce sky-link component

6b514b4

Fix styling

809143d

atmonshi reviewed Mar 18, 2024

View reviewed changes

mallardduck and others added 2 commits March 18, 2024 17:29

refactor add controls for classes

73b6cdc

Fix styling

b6c00b4

mallardduck changed the title ~~WIP: reimagine Navigation rendering features~~ Reimagine Navigation rendering features Mar 18, 2024

mallardduck and others added 2 commits March 21, 2024 14:33

refactor based on feedback

7bfddb3

Fix styling

6b0df43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimagine Navigation rendering features #199

Reimagine Navigation rendering features #199

mallardduck commented Mar 14, 2024 •

edited

Loading

mallardduck commented Mar 14, 2024

mallardduck left a comment

mallardduck Mar 14, 2024

mallardduck Mar 14, 2024

atmonshi Mar 18, 2024

atmonshi commented Mar 14, 2024

mallardduck commented Mar 14, 2024

atmonshi commented Mar 14, 2024

mallardduck commented Mar 14, 2024

mallardduck commented Mar 18, 2024 •

edited

Loading

atmonshi Mar 18, 2024

mallardduck Mar 18, 2024

atmonshi Mar 18, 2024

atmonshi commented Mar 18, 2024


		abstract public function getModel(): ?Model;

		abstract public function getLink(): ?string;

Reimagine Navigation rendering features #199

Are you sure you want to change the base?

Reimagine Navigation rendering features #199

Conversation

mallardduck commented Mar 14, 2024 • edited Loading

Motivation

New Feature Usage Example:

Further customizing the Anchor link:

TODO:

mallardduck commented Mar 14, 2024

mallardduck left a comment

Choose a reason for hiding this comment

mallardduck Mar 14, 2024

Choose a reason for hiding this comment

mallardduck Mar 14, 2024

Choose a reason for hiding this comment

atmonshi Mar 18, 2024

Choose a reason for hiding this comment

atmonshi commented Mar 14, 2024

mallardduck commented Mar 14, 2024

atmonshi commented Mar 14, 2024

mallardduck commented Mar 14, 2024

mallardduck commented Mar 18, 2024 • edited Loading

atmonshi Mar 18, 2024

Choose a reason for hiding this comment

mallardduck Mar 18, 2024

Choose a reason for hiding this comment

atmonshi Mar 18, 2024

Choose a reason for hiding this comment

atmonshi commented Mar 18, 2024

mallardduck commented Mar 14, 2024 •

edited

Loading

mallardduck commented Mar 18, 2024 •

edited

Loading