Bootstrap 3 media object #17
Conversation
nadar
left a comment
nadar
left a comment
There was a problem hiding this comment.
In general i appreciate this solution, as it sticks more to bootstrap styles. The only thing which could be against this change: There is no option for the margin anymore (cfgValue: margin)
| 'width' => $this->cfgValue('width', null), | ||
| 'height' => $this->cfgValue('height', null), | ||
| 'style' => (($this->varValue('imagePosition', 'left') == 'left') ? "margin-right:{$this->cfgValue('margin', '20px')}" : "margin-left:{$this->cfgValue('margin', '20px')}") . $this->cfgValue('margin', '20px', ';margin-bottom:{{margin}};'), | ||
| 'style' => $this->extraValue('imagePosition') . $this->cfgValue('margin', '20px', ';margin-bottom:{{margin}};'), |
There was a problem hiding this comment.
The 20px is just the default value. Do u think it is better to make a constant for it?
There was a problem hiding this comment.
I thought there is a class like m-l-2 like in the BS4 and others but there is not..
maybe just leave it like that, sry didin´t used BS3 for a long time.
In general, it´s always good to avoid hardcoded values, but your PR makes it better than before .. thank you. for me it´s fine from FE side
|
I am good, lets wait for @TheMaaarc (https://github.com/luyadev/luya/blob/master/docs/misc/organisation.md#6-roles ;-)) |
|
@boehsermoe its my fault, but now all unit tests are failing due to changed code. But as there is no CI server setup, we missed this point. |
4 participants
https://getbootstrap.com/docs/3.3/components/#media-default