-
Notifications
You must be signed in to change notification settings - Fork 4
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
OEL-1552: Theme the person content type #162
Conversation
templates/content/node--oe-sc-person--oe-w-content-banner.html.twig
Outdated
Show resolved
Hide resolved
templates/content/field--oe-document-reference--oe-document--oe-document.html.twig
Show resolved
Hide resolved
templates/content/field--oe-document-reference--oe-document-group.html.twig
Outdated
Show resolved
Hide resolved
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.
A little bit of feedback.
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
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.
Let's make sure we followup on why the email icon for the socials is not showing. I think we need the BCL update for that, so it's fine for now.
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Show resolved
Hide resolved
...ter_person/config/install/core.entity_view_display.node.oe_sc_person.oe_w_content_banner.yml
Outdated
Show resolved
Hide resolved
...ter_person/config/install/core.entity_view_display.node.oe_sc_person.oe_w_content_banner.yml
Outdated
Show resolved
Hide resolved
templates/content/field--oe-document-reference--oe-documents--oe-document-group.html.twig
Outdated
Show resolved
Hide resolved
templates/content/field--oe-document-reference--oe-document--oe-document.html.twig
Show resolved
Hide resolved
templates/content/field--oe-document-reference--oe-documents--oe-document-group.html.twig
Outdated
Show resolved
Hide resolved
37be31f
to
b79a04c
Compare
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.
We have some work to do in a followup to prevent showing field labels when access to media entities is removed, but for now it looks good, just one comment in the tests.
*/ | ||
#} | ||
{% for item in items %} | ||
<div{{ not loop.last ? ' class="mb-3"' }}"> |
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.
Left-over double quote.
<div{{ not loop.last ? ' class="mb-3"' }}"> | |
<div{{ not loop.last ? ' class="mb-3"' }}> |
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.
As above:
Why do we need this div with mb-3 at all?
The documents already have mt-4.
On the other hand: Imo this mt-4 from pattern-file.html.twig is questionable, so we could discuss if we really need it.
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.
I add because it was in the ewcms template and francesco ask to do the same.
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.
I remove the left-over double quote.
For the class you mention, I follow what Francesco said about to take the same ewcms has. I think that as long as we don't have the final designs we can leave it like this
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.
I would need to look at ewcms, no time for this unless you provide a link :)
But ok to keep until we get updated design.
templates/content/field--oe-document-reference--oe-document--oe-document.html.twig
Outdated
Show resolved
Hide resolved
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
...ter_person/config/install/core.entity_view_display.node.oe_sc_person.oe_w_content_banner.yml
Outdated
Show resolved
Hide resolved
...ter_person/config/install/core.entity_view_display.node.oe_sc_person.oe_w_content_banner.yml
Outdated
Show resolved
Hide resolved
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
templates/content/field--oe-document-reference--oe-document--oe-document.html.twig
Outdated
Show resolved
Hide resolved
templates/content/field--oe-document-reference--oe-title--oe-document-group.html.twig
Outdated
Show resolved
Hide resolved
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
if (!$access->isAllowed()) { | ||
$cacheability->applyTo($variables); |
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.
(tbd, don't implement directly)
Now we call this 3x, before each return.
We are not adding anything else to the $cacheability
, so we could do it just once and avoid code repetition.
if (!$access->isAllowed()) { | |
$cacheability->applyTo($variables); | |
$cacheability->applyTo($variables); | |
if (!$access->isAllowed()) { |
and then remove the other calls below.
Of course this only works if we don't add anything else to the $cacheability
later.
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.
We have to change this back once we introduce image styles.
So maybe keep it for now? @drishu
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.
Any news here @drishu @donquixote
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.
Keep it for next reviewer.
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.
agreed @donquixote, we need to code for what we have, and lets also remove the empty line 70
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.
I'm not sure but IMHO, this is not a blocker for this task, we have more or less the same in all the CT, so we should create a new ticket and do the fix everywhere and not block this ticket anymore, it's a ticket with a high priority to do this improvement now.
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
#} | ||
{% set _content %} | ||
{{ content.oe_summary|field_value }} | ||
<div class="mt-2-5 mb-2"> |
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.
Please check if social_links
is empty. Otherwise we get an empty div here.
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.
The condition needs to be outside the wrapper div.
templates/content/node--oe-sc-person--oe-w-content-banner.html.twig
Outdated
Show resolved
Hide resolved
templates/content/field--oe-document-reference--oe-document--oe-document.html.twig
Outdated
Show resolved
Hide resolved
templates/content/field--oe-document-reference--oe-document--oe-document.html.twig
Outdated
Show resolved
Hide resolved
*/ | ||
#} | ||
{% for item in items %} | ||
<div{{ not loop.last ? ' class="mb-3"' }}"> |
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.
As above:
Why do we need this div with mb-3 at all?
The documents already have mt-4.
On the other hand: Imo this mt-4 from pattern-file.html.twig is questionable, so we could discuss if we really need it.
*/ | ||
#} | ||
{% for item in items %} | ||
<h3 class="fs-4 mt-3 mb-2">{{ item.content }}</h3> |
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.
Currently the title is closer to the document or group above than the one it acts as a label for.
What should we do here? TBD.
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.
Let's keep it until we get an updated spec for these document groups.
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
modules/oe_whitelabel_starter_person/oe_whitelabel_starter_person.module
Outdated
Show resolved
Hide resolved
{# | ||
/** | ||
* @file | ||
* Field template of the oe_document_reference for the oe_document document. |
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.
Still awkward, but I don't have a good counter proposal.
Let's keep it, unless @drishu has a better idea during second review.
(see also the other field templates)
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.
My proposal is less scientific but more humane and understandable
"Field template of the Document reference entity."
We use the label not machine name this way we can understand.
But @donquixote has to agree first, if not we leave as is
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.
Actually this was Carlos first version, just in lowercase.
My initial request here was to include the field name, bundle and entity type, so we describe what we target.
But it is hard to put that into a single line.
One option would be to put these explanations in a separate block below:
* Field template of the oe_document_reference for the oe_document document. | |
* Template for the 'Document' field in the 'Document reference' entity type. | |
* | |
* Entity type: 'oe_document_reference'. | |
* Field: 'oe_document'. | |
* Bundle: 'oe_document'. |
Another option is to keep the comment simple and not have it include all the info, and instead we expect the reader to check the template name.
Tbh, I would not trust the comment anyway.
But, here, we actually can squeeze it into one line!
* Field template of the oe_document_reference for the oe_document document. | |
* Template for the 'Document' field in the 'Document' document reference type. |
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.
ok
$files->filter('p')->text() | ||
); | ||
|
||
$this->assertCount(3, $files); |
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.
The assertions for document group don't go very deep. But maybe it's enough.
Let's keep it unless @drishu complains in next review.
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.
#} | ||
{% set _content %} | ||
{{ content.oe_summary|field_value }} | ||
<div class="mt-2-5 mb-2"> |
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.
The condition needs to be outside the wrapper div.
if (!$access->isAllowed()) { | ||
$cacheability->applyTo($variables); |
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.
Keep it for next reviewer.
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.
github is weird.
I just submitted the review only by refreshing the browser window, without using the control at the top right.
Anyway, the comments do appear so let's go.
{# | ||
/** | ||
* @file | ||
* Field template of the oe_document_reference for the oe_document_group document. |
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.
* Field template of the oe_document_reference for the oe_document_group document. | |
* Template for 'Documents' field in 'Document group' document reference type. |
(omitting the 'the' to keep the line character limit)
{# | ||
/** | ||
* @file | ||
* Field template of the oe_document_reference for the oe_document_group title. |
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.
* Field template of the oe_document_reference for the oe_document_group title. | |
* Template for the 'Title' field in 'Document group' document reference type. |
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.
Let's follow up on the remaining issues
…improve node examples generation.
…inks from template.
…hen create Person node with all fields.
…ks are not empty before printing.
…on to the same file.
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.
Approved, we convened on follow up tickets
No description provided.