(feat): add image focal point component + logic #5
Conversation
Composer package changes
|
|
Coverage report for commit: 4b9aa01 Summary - Lines: - | Methods: -
🤖 comment via lucassabreu/comment-coverage-clover |
||||||||||||||||||||||||||||||||||||
laravdiemen
left a comment
laravdiemen
left a comment
There was a problem hiding this comment.
Misschien dat ik nog gewoon aan al die componenten moet wennen maar voor mijn gevoel wordt het zo meer beperkt. Waarom is er niet voor gekozen om alleen die trait (https://github.com/yardinternet/brave/blob/main/web/app/themes/sage/app/Data/Traits/FocalPoint.php) aan bijvoorbeeld Yard\Data\PostData standaard toe te voegen?
| @@ -0,0 +1,7 @@ | |||
| <img | |||
There was a problem hiding this comment.
Heb je een voorbeeld hoe dit dan in de html eruit komt te zien in de card-default bijvoorbeeld?
Dat is nu dit:
<img
class="card-image {{ $hasLogo ? 'card-image-logo !size-8 !object-contain' : '' }} aspect-[inherit] size-full object-cover"
src="{!! $hasLogo ? get_theme_file_uri('/resources/images/logo-element.svg') : $thumbnailUrl !!}"
alt=""
loading="lazy"
style="{{ $focalPoint }}"
>
There was a problem hiding this comment.
Je kan dan de class en src overschrijven:
<x-brave-img-focal-point
:id="$postData->id"
class="card-image {{ $hasLogo ? 'card-image-logo !size-8 !object-contain' : '' }} aspect-[inherit] size-full object-cover"
src="{!! $hasLogo ? get_theme_file_uri('/resources/images/logo-element.svg') : $thumbnailUrl !!}" />
src/Components/ImgFocalPoint.php
Outdated
| string $alt = '', | ||
| ) { | ||
| $this->class = $class; | ||
| $this->src = $this->getImageURL($id, $size); |
There was a problem hiding this comment.
Kan je dit ook gewoon zelf meegeven? We hebben bijvoorbeeld in card-default een logica dat er soms een logo element getoond moet worden
There was a problem hiding this comment.
Heb het aangepast dat als er nu een src wordt meegegeven in de component dat deze overschreven wordt.
src/Components/ImgFocalPoint.php
Outdated
|
|
||
| return sprintf( | ||
| '%s:%d%% %d%%;', | ||
| 'object-position', |
There was a problem hiding this comment.
Nu kunnen we ook een parameter meegeven om background-position te krijgen ipv object-position. Ik weet even niet waarom we dit gedaan hebben, maar het belangrijkste is denk ik, kunnen we deze functie ook nog gebruiken zonder dat je dat img-focal-point component moet gebruiken? Ik twijfel of het nu meer beperkt wordt
There was a problem hiding this comment.
Dit kan aangepast worden, bij het aanroepen van de component zou je een position attribute mee kunnen geven om de default te overschrijven:
string $position = 'object-position'
) {
$this->focalPoint = $this->calculateFocalPoint($id, $position);
return sprintf(
'%s:%d%% %d%%;',
$position,
$focalPoint['x'] * 100,
$focalPoint['y'] * 100
);
There was a problem hiding this comment.
ImgFocalPoint is gewoon een class. Dus als je ooit het focalpoint nodig hebt zonder het component kan je die zo ophalen:
$focalpoint = (new \Yard\Brave\Components\ImgFocalPoint($postId))->focalPoint;
Wat zou je met het focalpoint willen doen wat je nu niet met het component kan doen? Ik wil de trait niet aan de Yard\Data\PostData package toevoegen omdat het focalpoint daar niet thuis hoort. PostData moet enkel data objecten bevatten voor WordPress standaard objecten (post, meta, user, taxonomy, term etc.). |
Goed punt van die Yard\Data\PostData package. Het enige waar ik over twijfel is als je een wordpress blok moet nabouwen in een blade template voor bijvoorbeeld een singel page. Bijvoorbeeld de media-text heeft soms inline style in plaats van een gewone
|
|
En zou je er voor willen zorgen dat de phpstan action slaagt? Mag best op een laag level staan maar dan hebben we iets werkend. |
No description provided.