UCPKN-3056: Agenda theming. #268
Conversation
kp77
force-pushed
the
contribution/UCPKN-3056
branch
from
cb8621b
to
fc6918d
Compare
kp77
force-pushed
the
contribution/UCPKN-3056
branch
from
fc6918d
to
643ea79
Compare
kp77
force-pushed
the
contribution/UCPKN-3056
branch
from
3d73618
to
11664e0
Compare
|
Hi @msnassar, I added 3 new commits since last review round:
|
| * @see ./modules/contrib/oe_agenda/templates/oe-agenda-session.html.twig | ||
| */ | ||
| #} | ||
| {% macro icon(name = 'clock', size = 'xs', attributes) %} |
There was a problem hiding this comment.
I understand the reason behind assigning default values to the parameters. But I am not sure we should do this. As this macro is generic, I prefer to have them passed instead.
Also, I believe it is better to add macros to the end of the file...
There was a problem hiding this comment.
This macro is added to the session template, and the default icon we use in the session is what the defaults of the macro are assigning. What is the reason not to assign default values for the macro and making it easier to insert the default session icon for hours?
Regarding where to put the macro: I would have also put it to the end of the file, but the only macro examples I found in Whitelabel are placing the macro at the top of the file. Not sure if it's 'better' to put it to the end, I think it's just a question of preference/convention, and I was trying to follow the Whitelabel practice.
There was a problem hiding this comment.
Yes, it is part of the session template... But if we have another icons that we would like to display, I think we will use the same macro but in this case we will pass values to the parameters!
The reasons I prefer not to set default values:
- Code readability.
{{ _self.icon() }}doesn't tell much! specially if we will use the macro to display other icons (which is not the case now). - Less confusing for other developers who would like to extend/override the template and also reuse the macro.
Anyway. lets see the opinion from the other reviewers ;)
There was a problem hiding this comment.
Well, for me {{ _self.icon() }} tells something: it tells that there's a macro in this template (or the parent template) that defines an icon for the template. If I don't know what it outputs and how, I should probably go and check it.
The original purpose of this macro is to output the icon we need - as we need it - in multiple places without repeating ourselves. If I have to pass everything in the different places I need the icon (and that includes also the spacing classes passed in the attributes), that goes against the original idea of using the macro (I might as well just call the pattern twice..)
The fact that this macro also can be used to output other icons in other sizes/attributes is just a bonus IMO.
There was a problem hiding this comment.
Checking core and other modules/themes, there are never defaults. Let's not use them.
| foreach (['oe_session_venue', 'oe_session_online_link'] as $index => $field_name) { | ||
| $field = $session->get($field_name); | ||
| assert($field instanceof FieldItemListInterface); | ||
| if ($field->isEmpty() || !$field->access()) { |
There was a problem hiding this comment.
We need to gather the access cache metadata and bubble it up here.
kp77
force-pushed
the
contribution/UCPKN-3056
branch
from
737e683
to
1c53d69
Compare
With the ER fields the isEmpty() check is not reliable in case the referenced entity is deleted.
kp77
force-pushed
the
contribution/UCPKN-3056
branch
from
1c53d69
to
c4616ab
Compare
|
Hi @brummbar, The changes since your last review are all commits after and including: UCPKN-3056: Add session link fields access cache metadata to render array. Please have a look. |
Hi @brummbar,
The PR is still wip, without tests. There are some changes since we last discussed, most importanty that I ended up adding templates/preprocesses only for the bundles that are shipped by oe_agenda, and I don't override the general entity templates. I think it's cleaner like this, because we use bundle fields in all the templates.
This affected the session templates a bit, so now I don't have a
oe-agenda-session.html.twigthat theoe-defaultandoe-breaktemplates are extending, instead theoe-breaktemplate extends theoe-default(but otherwise the idea is the same). Let me know what you think, I will add comments to the code where I have something to say.