Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
OEL-1552: Theme the person content type #162
Changes from all commits
668d5cf
7caf38c
4af6f4d
3dd3efe
196b265
3693a7b
3dca9f3
afc522c
68c72a7
020afb3
e5928dc
6aee466
24d8e35
a4a2afa
057606a
1a20be1
9cb71cd
06e6da7
1456804
87ad166
c17bf97
5714491
a0a6c14
6f89136
3378b3c
3b3e96e
aa67fb7
e7805c5
5109695
6764448
08934f0
674b48e
5df5bb0
f647d03
9982874
4367307
e276db7
5ddd4a2
a63fe2a
65df3ca
c6f816a
a82a6d4
6afe15e
566f8d0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.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.
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:
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!
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