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

Small Improvements #84

Merged
merged 2 commits into from
Feb 15, 2024
Merged

Conversation

saade
Copy link
Contributor

@saade saade commented Feb 15, 2024

Hey! Thanks for your plugin!

I've started using it today, and wanted to contribute with some small improvements.

JS params

Parameters were being passed inside a string, this was causing boolean values to be passed as '1' and '0'

Before After

Flickering

Although a small flicker still occurs, its much better than before. I think the flickering can't be avoided, but now it has the same flickering as the Filament's Core Select Input

Before After
Screen.Recording.2024-02-15.at.14.04.40.mov
Screen.Recording.2024-02-15.at.14.04.50.mov

Alpine clashing with other plugins

This is not this plugin fault, filament-adjacency-list uses a tree() named alpine component too, this causes our plugins to not be compatible when used together in the same form.

I've renamed mine to adjacencyList() but wanted to contribute to yours too, since tree() is too generic, i've renemed yours to selectTree().

Asset renaming and merging

I've merged your two assets tree and custom into a index one. Your plugin now ships with only one css file and only one request is made to grab the asset. This helps poor connections to load the plugin faster since its only one file to request.

Other

  • Code formatting

There is no BC in this PR, feel free to review and ask for changes. Thanks!

@CodeWithDennis
Copy link
Owner

Hey, thank you for your help! I can't recall the exact reason, but I had to input it as a string. If you could confirm that everything is still functioning as intended im fine with it!. @saade

@saade
Copy link
Contributor Author

saade commented Feb 15, 2024

@CodeWithDennis Here's the checklist:

  • searchable()
  • withCount()
  • placeholder()
  • enableBranchNode()
  • disabled()
  • independent()
  • alwaysOpen()
  • clearable()
  • emptyLabel()
  • expandSelected()
  • grouped()
  • defaultOpenLevel()
  • direction()
  • rtl()

@saade
Copy link
Contributor Author

saade commented Feb 15, 2024

Seems everything is covered 👍🏼

@CodeWithDennis CodeWithDennis merged commit f6bf2f6 into CodeWithDennis:3.x Feb 15, 2024
1 check passed
@saade saade deleted the feat/small-improvements branch February 15, 2024 17:58
@mohamedsabil83
Copy link
Contributor

Thanks, @CodeWithDennis, for the helpful package and @saade, for the valuable contributions.

After this PR, the $set method is not applied to update select-tree value anymore:

->hintAction(fn () => Forms\Components\Actions\Action::make('add_category')
    ->label(__('Add Category'))
    ->icon('heroicon-m-plus')
    ->action(function ($component, array $data, $form, Get $get, Set $set): void {
        $record = $component->getRelationship()->getRelated();
        $record->fill($data);
        $record->save();

        $set('shop_category_id', [...Arr::wrap($get('shop_category_id')), $record->getKey()]);
    })
    ->form([
        Forms\Components\TextInput::make('name')
            ->label(__('validation.attributes.name'))
            ->required()
            ->rule(fn ($livewire) => UniqueJsonRule::for(table: 'shop_categories', column: 'name', locale: $livewire->activeLocale)),
        SelectTree::make('parent_id')
            ->label(__('validation.attributes.parent_category'))
            ->relationship(
                relationship: 'category',
                titleAttribute: 'name',
                parentAttribute: 'parent_id',
                modifyQueryUsing: fn ($query) => $query
            )
            ->enableBranchNode()
            ->defaultOpenLevel(10),
    ])
)

When removing wire:ignore & x-ignore, the new value was selected, but the field disappeared after.

I comment here, but I can post a new issue if you'd like.

@saade
Copy link
Contributor Author

saade commented Feb 17, 2024

Thats strange, it was already there before 🤔 I'll take a look later tonight

@saade saade mentioned this pull request Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants