-
Notifications
You must be signed in to change notification settings - Fork 1
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
Lightbox to header, add tooltip to buttons and small styling adjustments (Meta #119) #63
Conversation
15828d1
to
965e20d
Compare
Strip conditional classes and attributes out of element declaration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcwieland95 lgtm for now.
{% macro add_button(content, view, isOnShow) %} | ||
{% set createUrl = content.createUrl(view.data) %} | ||
{% if createUrl and (content.addVoterAttribute is null or is_granted(content.addVoterAttribute, view.data)) %} | ||
|
||
|
||
{% set additionalClass = (not isOnShow) ? (content.option('attr')['class'] ?? '') : '' %} | ||
{% set additionalAttributes = (not isOnShow) ? attr|map((value, attr) => "#{attr}=\"#{value}\"")|join(' ') : '' %} | ||
|
||
<div | ||
class="flex justify-end {{ additionalClass }}" | ||
{{ additionalAttributes|raw }} | ||
{{ stimulus_controller('araise/core-bundle/modal-form', { formUrl: createUrl }) }}> | ||
|
||
<button | ||
type="button" | ||
class="whatwedo-crud-button--action whatwedo-crud-button--rounded" | ||
{{ stimulus_action('araise/core-bundle/modal-form', 'openModal') }} | ||
{{ stimulus_controller('araise/crud-bundle/tooltip', { 'title': 'araise_crud.relation.add'|trans } ) }} | ||
> | ||
{{ bootstrap_icon('plus', { class: 'w-6 h-6' }) }} | ||
</button> | ||
|
||
<div | ||
{{ stimulus_target('araise/core-bundle/modal-form', 'menu') }} | ||
class="hidden whatwedo-crud-modal whatwedo-utility-modal" | ||
data-transition-enter-from="opacity-0" | ||
data-transition-enter-to="opacity-100" | ||
data-transition-leave-from="opacity-100" | ||
data-transition-leave-to="opacity-0" | ||
role="dialog" | ||
aria-modal="true" | ||
> | ||
<div class="whatwedo-crud-modal__overlay whatwedo-utility-modal__overlay"> | ||
<div | ||
class="whatwedo-crud-modal__backdrop whatwedo-utility-modal__backdrop" | ||
{{ stimulus_action('araise/core-bundle/modal-form', 'close', 'click') }} | ||
></div> | ||
|
||
{# This element is to trick the browser into centering the modal contents. #} | ||
<span class="hidden md:inline-block md:align-middle md:h-screen" aria-hidden="true">​</span> | ||
|
||
<div class="whatwedo-crud-modal__wrapper whatwedo-utility-modal__wrapper"> | ||
<div class="whatwedo-utility-modal__wrapper-inner"> | ||
<button | ||
type="button" | ||
class="whatwedo-crud-modal__close whatwedo-utility-modal__close" | ||
{{ stimulus_action('araise/core-bundle/modal-form', 'close') | stimulus_action('araise/core-bundle/modal-form', 'close', 'keydown.esc@window') }} | ||
> | ||
<span class="sr-only">Close</span> | ||
{{ bootstrap_icon('x', { class: 'inline w-8 h-8' }) }} | ||
</button> | ||
|
||
<div | ||
class="whatwedo-crud-modal__body w-full" | ||
{{ stimulus_target('araise/core-bundle/modal-form', 'modalBody') }} | ||
></div> | ||
</div> | ||
</div> | ||
</div> | ||
</div> | ||
</div> | ||
{% endif %} | ||
{% endmacro %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: That macro should at least go to the bottom of the file. But the best approach would be, putting it into a separate file and importing it.
The current approach works, but is the messy one.
Another Nit is that the macro is using snake_case and the variables are using camelCase. The official coding standards of Twig say to use snake_case (See paragrpah Use lower cased and underscored variable names
).
https://twig.symfony.com/doc/3.x/coding_standards.html
but this Nit is really out of scope as the overall code does not apply any coding standards and checks and is currently already mixed between snake_case and camelCase.
{% if table.exporters|length > 0 and table.option('definition') and table.option('definition').hasCapability(constant('araise\\CrudBundle\\Enums\\Page::EXPORT')) %} | ||
<div | ||
{{ stimulus_controller('araise/core-bundle/dropdown') }} | ||
class="relative inline-block"> | ||
<button | ||
{{ stimulus_action('araise/core-bundle/dropdown', 'toggle') | stimulus_action('araise/core-bundle/dropdown', 'close', 'keydown.esc') }} | ||
class="whatwedo_table:header__button whatwedo_table:header__button--download whatwedo_table-button h-full" tabindex="-1" | ||
{{ stimulus_controller('araise/crud-bundle/tooltip', { 'title': 'araise_table.download.tooltip'|trans } ) }} | ||
class="whatwedo_table:header__button whatwedo_table:header__button--download whatwedo_table-button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Still a button without a text or aria-label.
No description provided.