Skip to content
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

feat: added global search suffix #12549

Merged
merged 20 commits into from
May 3, 2024

Conversation

lukas-frey
Copy link
Contributor

@lukas-frey lukas-frey commented Apr 29, 2024

Description

This PR adds the ability to add and modify the suffix of the global search field (if global search is enabled).

By default, the newly added method getGlobalSearchSuffix() returns false. In this case it will behave like before the PR - no suffix will be rendered.

You can enable the suffix using the globalSearchSuffix() method on the panel:

use Filament\Panel;

public function panel(Panel $panel): Panel
{
    return $panel
        // ...
        ->globalSearchSuffix();
}

It accepts one parameter which can be of three types:

  • string: In this case, whichever string you pass into the method will also be rendered in the suffix.
  • Closure: In this case, you can run your own closure to determine which string to render. The closure will also pass an evaluation parameter called platform which contains the name of the platform parsed from the user-agent (described later on). This is useful if you want to display the key-binding differently for each platform, such as Ctrl+K on Windows, but ⌘K on Mac.
  • bool:
    • true: In this case, the package will use the first key-binding from the key-bindings array and use the platform that was parsed from the user-agent to determine what to display. It will replace some OS-specific keys with the substitutes on other OSes according to mousetrap's documentation.
    • false: In this case, it will simply be disabled (same as if the method was omitted).

Platform parsing from user-agent:
Currently there's a very simple check to determine the OS from the user-agent and currently it differentiates between:

  • Windows
  • Mac
  • Linux
  • Other

So if you wanted to customize the suffix for each platform, you could do the following:

use Filament\Panel;

public function panel(Panel $panel): Panel
{
    return $panel
        // ...
        ->globalSearchSuffix(fn(string $platform) => match($platform) {
            'Windows'|'Linux' => 'CTRL+K',
            'Mac' => '⌘K'
            default => null,
        });
}

Visual changes

Screenshot 2024-04-29 at 20 36 24

Functional changes

  • Code style has been fixed by running the composer cs command.
  • Changes have been tested to not break existing functionality.
  • Documentation is up-to-date.

@danharrin danharrin added the enhancement New feature or request label May 1, 2024
@danharrin danharrin added this to the v3 milestone May 1, 2024
Copy link
Member

@danharrin danharrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good ideas, thanks!

However, I feel like there are two features rolled into this one method at the moment.

Let's keep globalSearchFieldSuffix() (notice the name change) as just handling custom suffixes. Please rename the corresponding property and getter to match. The corresponding property can be nullable instead of accepting a bool, Then, a new method can be introduced as globalSearchKeyBindingsFieldSuffix(), which just calls ->globalSearchFieldSuffix() and passes a closure to return the keybindings based on the platform. Does that make sense?

@danharrin
Copy link
Member

Also, please refactor the platform to an enum.

@danharrin danharrin self-assigned this May 1, 2024
@lukas-frey
Copy link
Contributor Author

Also, please refactor the platform to an enum.

Will do :) Should this be an Enum in the Support package or the Panels package? I'm not sure whether there would be some use in other packages for it as well.

Good ideas, thanks!

However, I feel like there are two features rolled into this one method at the moment.

Let's keep globalSearchFieldSuffix() (notice the name change) as just handling custom suffixes. Please rename the corresponding property and getter to match. The corresponding property can be nullable instead of accepting a bool, Then, a new method can be introduced as globalSearchKeyBindingsFieldSuffix(), which just calls ->globalSearchFieldSuffix() and passes a closure to return the keybindings based on the platform. Does that make sense?

I see, makes sense! Shouldn't the method be rather called globalSearchFieldKeyBindingsSuffix()? :D I'm not a native english speaker, so I'll leave that decision up to you, but imo the first name evoked that it's some kind of KeyBinding Field.

And the platform evaluation parameter, that one should still be available also in the globalSearchFieldSuffix method, right?

@lukas-frey lukas-frey requested a review from danharrin May 2, 2024 09:06
@danharrin
Copy link
Member

Sure, globalSearchFieldKeyBindingsSuffix() sounds better probably

@danharrin
Copy link
Member

Also, can we have a new helper method in filament/support called detect_platform() which returns Filament\Support\Enums\Platform? Then we don't need to inject it explicitly, and it can be used anywhere in Filament.

@lukas-frey
Copy link
Contributor Author

lukas-frey commented May 2, 2024

Also, can we have a new helper method in filament/support called detect_platform() which returns Filament\Support\Enums\Platform? Then we don't need to inject it explicitly, and it can be used anywhere in Filament.

Sounds good. I've added it. But do I have to do something else on top of running composer dump-autoload? I'm having trouble finding the helper function and the enum from my testing sandbox project now that I moved it to the Support package.

@danharrin danharrin merged commit 9385064 into filamentphp:3.x May 3, 2024
10 checks passed
@danharrin
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants