Create dropup for profile #27

marcwieland95 · 2023-11-16T21:43:09Z

Frontend code for new "dropup" -> profile sidebar

araise/araise-meta#56

renestalder · 2023-11-17T13:46:47Z

src/Resources/views/base.html.twig

@@ -87,6 +87,10 @@
                            </div>
                        {% endblock %}
                    {% endblock %}
+
+                    {% block sidebar_profile %}
+                        {% include "@araiseCrud/includes/sidebar/_profile.html.twig" %}


Nit: Code-Style. Project should add a Code-Style checker for these things to not be Double Quotes for some cases and then again single quotes for other cases (see block('...') which are all single quotes).

renestalder · 2023-11-17T13:54:48Z

src/Resources/views/includes/sidebar/_profile.html.twig

+                {{ bootstrap_icon('pencil-square', { class: 'h-3 w-3 self-center mr-2', alt: '' }) }}
+                {% block profile_edit_text %}Profil bearbeiten{% endblock %}
+            <a
+                href="{% block logout_link %}{% endblock %}"


It's odd to use blocks like that. This file probably would be better off as a macro instead of a template, if it needs this level of flexibility. Otherwise, we should use {{ block('logout_link') }} instead.

The problem with this pattern here is that it is difficult to read. We can't make a readable, proper null/undefined check. And we also can't enforce the block to be set, as the blocks are optional. Yet the href must never be empty.

Way better as a Macro.

I created it inside the same file and when no macro is imported (just the file included), I added a text with a notification to change it for all projects which has it included differently

renestalder · 2023-11-17T13:56:12Z

src/Resources/views/includes/sidebar/_profile.html.twig

+                    <img class="inline-block h-4 w-4 rounded-full self-center mr-2"
+                         src="{% block profile_picture %}{% endblock %}" alt=""
+                         crossorigin="anonymous" referrerpolicy="no-referrer">
+                    {% block profile_fullname %}[email protected]{% endblock %}


Do not put static, invalid data as a default to the code. It will be forgotten and it will come up in production.

That's right. I didn't found where to get the data from the backend but wanted to create the MR before I'm in holiday. Sandro has a task to also create the images proper.

renestalder · 2023-11-17T14:02:12Z

src/Resources/views/includes/sidebar/_profile.html.twig

+        data-transition-leave-to="opacity-0 scale-95"
+        tabindex="-1"
+    >
+        <div class="flex flex-col gap-y-2 m-4 -mb-1 px-4 pt-2 pb-3 rounded-sm shadow-md z-10 bg-white ring-1 ring-black ring-opacity-5 focus:outline-none">


Based on the design spec, this is a dropdown with multiple options; therefore we'd expect the content to be a list of options (<ul> and <li>).

renestalder · 2023-11-17T14:03:53Z

src/Resources/views/includes/sidebar/_profile.html.twig

+        tabindex="-1"
+    >
+        <div class="flex flex-col gap-y-2 m-4 -mb-1 px-4 pt-2 pb-3 rounded-sm shadow-md z-10 bg-white ring-1 ring-black ring-opacity-5 focus:outline-none">
+            <div class="border-b">


Semantically, this would need to be a <p> for the content to make sense. A <div> has no semantic meaning, but is required in this case.

Based on the changes above the div with the class border-b is now a li.

renestalder · 2023-11-17T14:05:44Z

src/Resources/views/includes/sidebar/_profile.html.twig

+    {{ stimulus_controller('araise/core-bundle/dropdown', {'alignment': 'top'}) }}
+>
+
+    <button


Nit: I didn't check the dropdown controller, so please quickly verify there is either an aria-expanded or aria-pressed or similar marked added by it to ensure, the button meaning is correctly reflected in the accessibility tree.

No, there's nothing set like that. We should integrate this in the dropdown controller absolutely

Added to this issue: araise-dev/CoreBundle#14

marcwieland95 · 2023-12-11T15:04:34Z

@renestalder Can you check the updated version with the macro again? Thanks

renestalder

Logout and Profil bearbeiten are hard coded strings. Is there an i18n system built in that should be called on those string to make it possible to overwrite them with translations?

renestalder · 2023-12-12T09:23:52Z

src/Resources/views/includes/sidebar/_profile.html.twig

                class="flex whatwedo-utility-paragraph transition-colors hover:text-neutral-500"
            >
                {{ bootstrap_icon('box-arrow-right', { class: 'h-3 w-3 self-center mr-2', alt: '' }) }}
-                {% block logout_text %}Logout{% endblock %}
+                {{ 'Logout' }}


Nit: Wrapping into a twig expression is unnecessary here. Instead of {{ 'Logout' }}, use Logout.

renestalder · 2023-12-12T09:35:37Z

src/Resources/views/includes/sidebar/_profile.html.twig

@@ -1,3 +1,4 @@
+{% macro profile(name, edit_link = null, name_detail = null, picture_src = null) %}


Nit: As with components in other templating systems, we have to take architecture of a growing “component” system into consideration. I don't think there is a good, official styleguide. But here is something that I learned:

Separating configuration options from data.

Having a first parameter to be data, a second one to be configuration.

Having an easy way to extend data, therefore passing it as an object.

As macros grow, having all properties as individual parameters will not be effortless to maintain, as you will have several breaking changes to introduce along the way.

Based on this, the recommendation I would have for upcoming macros is, passing the data in a first parameter as an object, e.g. {{ profile({ name: name, edit_link: edit_link, picture_src: picture_src}) }}. This allows to update the data object parameters without changing the macro signature.

Second, if you have settings later like additional HTML Attributes, themes etc., this signature could look like this:

{% macro profile(data, opts) %} ....

{{ profile({ name: name, edit_link: edit_link, picture_src: picture_src }, { variant: 'compact', attr: { class: 'example' } ) }}

I also often look at the form macros of Symfony for inspiration.

https://symfony.com/doc/current/form/form_customization.html

Just some ideas to consider. Not a hard rule.

Good thinking. I changed the params to an object.

One thing which we can't do anymore is setting a default value and forcing an input. Do you know a solution to these issues? The name and link need to be set.

@marcwieland95

You set defaults like this, inside the macro:

{% set name = data.name|default('my-default') %}

Or if you want to mirror it into the incoming data object, you can do it as follows. This won't work 1:1 with nested data due to it now being shallow/deep merge. If you pass data that has nested objects, you need to manually use the merge-filter on the nested properties.

{% set default_data = { name: 'my-name' } %} {% set data = default_data|merge(data) %}

I recommend not to use merge on data inputs (only options/properties) and rather use the first approach. If data can be a PHP class instead of a raw array (which is possible), the merge-filter will throw an error. So if you assume the developers will pass PHP classes as data, do not try to use the merge filter directly on the data object.

src/Resources/views/includes/sidebar/_profile.html.twig

renestalder · 2023-12-12T09:40:17Z

@marcwieland95 Added a couple of nitpicking comments and a question about the hard-coded strings. But otherwise lgtm.

araise/araise-meta#56

…d easier to handle #27

marcwieland95 requested a review from renestalder November 16, 2023 21:43

marcwieland95 self-assigned this Nov 16, 2023

renestalder requested changes Nov 17, 2023

View reviewed changes

marcwieland95 force-pushed the feature/profile branch from ceb1b22 to c28d806 Compare December 5, 2023 02:12

renestalder approved these changes Dec 12, 2023

View reviewed changes

marcwieland95 added 9 commits December 18, 2023 21:28

feat: create dropup for profile

e6aa637

araise/araise-meta#56

style: don't use double quotes for include twig function

75840e1

fix: add margin top to button inside block

094425f

feat: move sidebar block into macro and make things easier to read an…

cc8090c

…d easier to handle #27

feat: use string translation and remove hard coded strings

0414504

feat: use data object and pass it to make it more future proof

76f1835

feat: also replace profile macro in base template

64e3ead

chore: passed fixed data to test

ce73a85

feat: pass logout link as well as param

ad2e1c5

marcwieland95 force-pushed the feature/profile branch from d5ba6ce to ad2e1c5 Compare December 18, 2023 20:35

marcwieland95 merged commit e36601c into develop Dec 18, 2023
1 check passed

marcwieland95 deleted the feature/profile branch December 18, 2023 20:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create dropup for profile #27

Create dropup for profile #27

marcwieland95 commented Nov 16, 2023

renestalder Nov 17, 2023

renestalder Nov 17, 2023

marcwieland95 Dec 11, 2023

renestalder Nov 17, 2023

marcwieland95 Nov 17, 2023

renestalder Nov 17, 2023

renestalder Nov 17, 2023

marcwieland95 Dec 11, 2023

renestalder Nov 17, 2023

marcwieland95 Nov 17, 2023

marcwieland95 Dec 11, 2023

marcwieland95 commented Dec 11, 2023

renestalder left a comment •

edited

Loading

renestalder Dec 12, 2023

renestalder Dec 12, 2023 •

edited

Loading

marcwieland95 Dec 18, 2023

renestalder Dec 20, 2023

renestalder commented Dec 12, 2023

		@@ -1,3 +1,4 @@
		{% macro profile(name, edit_link = null, name_detail = null, picture_src = null) %}

Create dropup for profile #27

Create dropup for profile #27

Conversation

marcwieland95 commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcwieland95 commented Dec 11, 2023

renestalder left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renestalder Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renestalder commented Dec 12, 2023

renestalder left a comment •

edited

Loading

renestalder Dec 12, 2023 •

edited

Loading