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

Init Admin ui package #20

Merged
merged 2 commits into from
Sep 15, 2024
Merged

Init Admin ui package #20

merged 2 commits into from
Sep 15, 2024

Conversation

loic425
Copy link
Member

@loic425 loic425 commented Jul 2, 2024

No description provided.

@loic425 loic425 force-pushed the init-admin-ui branch 4 times, most recently from c500762 to 4abebdf Compare July 2, 2024 07:25
@loic425 loic425 force-pushed the init-admin-ui branch 2 times, most recently from e7bae9d to 9ea0f99 Compare September 4, 2024 09:51
@loic425 loic425 force-pushed the init-admin-ui branch 2 times, most recently from 8561460 to ecf497f Compare September 13, 2024 06:39
{
}

public function createMenu(array $options): ItemInterface
Copy link
Member Author

Choose a reason for hiding this comment

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

The default menu is empty, we have to decorate it in our end-user application.


public function create(HookMetadata $hookMetadata, DataBagInterface $context, ScalarDataBagInterface $configuration, array $prefixes = []): HookableMetadata
{
$context['routing'] = $this->routing;
Copy link
Member Author

Choose a reason for hiding this comment

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

We pass the routing (pathes) configuration to all the templates.

Copy link
Member Author

Choose a reason for hiding this comment

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

This following routing configuration will be passed to the template.

sylius_admin_ui:
    routing:
        dashboard_path: '/admin'

@loic425 loic425 force-pushed the init-admin-ui branch 4 times, most recently from 1e92ba1 to eead120 Compare September 13, 2024 08:42
)
->addActionGroup(
ItemActionGroup::create(
// ShowAction::create(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// ShowAction::create(),

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'll have to build a default show template as the internal core team did in the 2.0 branch.

;
}

public function getResourceClass(): string
Copy link
Member

Choose a reason for hiding this comment

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

I understand it comes from grid configuration, but conceptually it's strange it's not a static method.

---     public function getResourceClass(): string
+++     public static function getResourceClass(): string

Copy link
Member Author

@loic425 loic425 Sep 15, 2024

Choose a reason for hiding this comment

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

In the first iteration of the grid, it was a static method, but we remove that static in order to use the class model parameter from Sylius.

in this example:
%app.model.book.class%

$services->alias(MenuBuilderInterface::class, 'sylius_admin_ui.knp.menu_builder');

$services->set('sylius_admin_ui.twig_hooks.factory.hookable_metadata', RoutingHookableMetadataFactory::class)
->decorate('twig_hooks.factory.hookable_metadata')
Copy link
Member

Choose a reason for hiding this comment

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

Are there plans to fix namespace for twig_hooks to sylius_twig_hooks since it starts to affect related packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

we have to discuss about it in the core team channel, but I agree, it's a bit risky to have no sylius prefix.

self::assertResponseIsSuccessful();
}

public function testAddingBookContent(): void
Copy link
Member

Choose a reason for hiding this comment

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

Naming wise

    public function testAddingBookContent(): void
    public function testEditingBookContent(): void

is a bit confusing since it tests page existence, while name suggests actual add/edit actions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I know, cause for now it's just "empty", but later we'll test the content of the adding and editing page.

Copy link
Member Author

@loic425 loic425 Sep 15, 2024

Choose a reason for hiding this comment

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

    public function testAddingBookContent(): void
    {
        $this->client->request('GET', '/admin/books/new');

        self::assertResponseIsSuccessful();

        // Validate Header
        self::assertSelectorTextContains('h1.page-title', 'New Book');

        // Validate Form
        self::assertSelectorTextContains('label[for=sylius_resource_title]', 'Title');
        self::assertSelectorExists('#sylius_resource_title');

        self::assertSelectorTextContains('label[for=sylius_resource_authorName]', 'Author name');
        self::assertSelectorExists('#sylius_resource_authorName');

        // Validate Buttons
        self::assertSelectorExists('[type="submit"]:contains("Create")');
        self::assertSelectorExists('[data-test-cancel-changes-button]');
    }

@loic425 loic425 merged commit 9213b11 into Sylius:main Sep 15, 2024
6 checks passed
@loic425 loic425 deleted the init-admin-ui branch September 15, 2024 13:16
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.

2 participants