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

Add Login page to the new Admin Panel #15352

Conversation

jakubtobiasz
Copy link
Member

Q A
Branch? bootstrap-admin-panel (2.0)
Bug fix? no
New feature?
BC breaks?
Deprecations?
Related tickets based on #15350
License MIT
CleanShot 2023-09-22 at 15 43 57@2x

@jakubtobiasz jakubtobiasz added Admin AdminBundle related issues and PRs. Frontend Issues and PRs related to frontend labels Sep 22, 2023
@jakubtobiasz jakubtobiasz requested a review from a team as a code owner September 22, 2023 13:44
@jakubtobiasz jakubtobiasz force-pushed the bootstrap-admin-panel/add-login-page branch from f10468a to 2cecce2 Compare September 22, 2023 13:44
@github-actions
Copy link

github-actions bot commented Sep 22, 2023

Bunnyshell Preview Environment deleted

Available commands:

  • /bns:deploy to redeploy the environment

@@ -0,0 +1,3 @@
{% import '@SyliusAdmin/Shared/Helper/button.html.twig' as _button %}

{{ _button.primary({ text: 'sylius.ui.login'|trans, type: 'submit', class: 'btn btn-primary w-100' }) }}
Copy link
Member

Choose a reason for hiding this comment

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

I'm out of the loop on this, why there is _ prefix for _button?

Copy link
Member Author

Choose a reason for hiding this comment

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

I use _ to indicate it's something imported. Same as I later started to use _context.<var> instead of just <var> to indicate from where something is coming. IMO it helps to understand from where some value is taken.

@@ -0,0 +1,10 @@
{% macro default(img, content, class) %}
Copy link
Member

@diimpp diimpp Sep 25, 2023

Choose a reason for hiding this comment

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

Is there something wrong with old Macro namespace?

I'm not a against a rename per-se, but very much against Helper namespace anywhere. It's one of those Util/Helper/Service dumpster dirs, which doesn't indicate content.

--- views/Shared/Helper/avatar.html.twig
+++ views/Shared/Macro/avatar.html.twig

<div class="input-icon">
<input type="text" placeholder="Search..." aria-label="Search in website" class="form-control ">
<span class="input-icon-addon">
<svg xmlns="http://www.w3.org/2000/svg" class="icon icon-tabler" width="24" height="24" viewBox="0 0 24 24" stroke-width="2" stroke="currentColor" fill="none" stroke-linecap="round" stroke-linejoin="round">
Copy link
Member

Choose a reason for hiding this comment

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

Should it use icon macro?

@@ -0,0 +1,12 @@
<form action="#" method="get" autocomplete="off" novalidate="">
<div class="input-icon">
<input type="text" placeholder="Search..." aria-label="Search in website" class="form-control ">
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
<input type="text" placeholder="Search..." aria-label="Search in website" class="form-control ">
<input type="text" placeholder="Search..." aria-label="Search in website" class="form-control">

placeholder needs to be translated

@@ -0,0 +1 @@
{{ form_row(form._password) }}
Copy link
Member

Choose a reason for hiding this comment

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

Is form._password some kind of special syntax of form element simply named this way?

use Symfony\Contracts\Translation\TranslatorInterface;
use Symfony\UX\TwigComponent\Attribute\ExposeInTemplate;

final class UserDropdown
Copy link
Member

Choose a reason for hiding this comment

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

Just an idea,

Suggested change
final class UserDropdown
final class UserMenu
final class UserMenuDropdown
final class NavbarSecurity

Main takeaway from this, that we should establish "public" naming scheme for components, e.g. content based, element based, etc

{% set event_name = _context.event_prefix ~ '.form' %}

{{ form_start(form, {'action': path('sylius_admin_login_check'), 'attr': {'class': 'ui large loadable form', 'novalidate': 'novalidate'}}) }}
{{ sylius_template_event(event_name, _context|merge({ event_prefix: event_name })) }}
Copy link
Member

Choose a reason for hiding this comment

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

This action repeats several times _context|merge({event_prefix: event_name}), might be worth handling at sylius_template_event side or via macro

@@ -0,0 +1,12 @@
<form action="#" method="get" autocomplete="off" novalidate="">
Copy link
Member

Choose a reason for hiding this comment

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

No action yet

@@ -0,0 +1,7 @@
{% form_theme form 'bootstrap_5_layout.html.twig' %}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it use intermediate sylius layout and not bootstrap directly?

Copy link
Member

Choose a reason for hiding this comment

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

Common was the indication of Shared in the old templates.
Is it renamed temporarily to separate old/new templates or Shared is new Common, if so, then why?

Navbar for Crud is already common without clarification. For example show's specific navbar will be simply put at Shared/Crud/Show/Navbar

--- Shared/Crud/Common/Navbar
+++ Shared/Crud/Navbar

@jakubtobiasz jakubtobiasz merged commit 444f3ee into Sylius:bootstrap-admin-panel Nov 10, 2023
11 of 12 checks passed
@jakubtobiasz jakubtobiasz deleted the bootstrap-admin-panel/add-login-page branch November 10, 2023 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin AdminBundle related issues and PRs. Frontend Issues and PRs related to frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants