Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
this doesn't look quite right; please give me the weekend to review |
jbergstroem
left a comment
jbergstroem
left a comment
There was a problem hiding this comment.
blocking so it doesn't land as-is
|
I do agree with removing the height and width argument (but it shoudl be easy to retrieve) but my concern is with how opinionated this becomes. I would rather see custom css being passed as-is or just making it easy to target via a css rule so styling can fit into an existing css system. |
|
Keep in mind: removing width/height is semver breaking; we need to make sure we don't mess with existing installations. |
anonvt
force-pushed
the
feat/frank/css-styling
branch
from
48e9031 to
15d8c2f
Compare
|
@jbergstroem I've reinterpreted this PR |
| width?: number, | ||
| height?: number, | ||
| bannerClass?: string, | ||
| ): TemplateResult { |
There was a problem hiding this comment.
don't change the order of variables, thats a breaking change
| src="${src}" | ||
| width="${width}px" | ||
| height="${height}px" | ||
| styles=${mediaStyle} |
There was a problem hiding this comment.
im generally ok with this if there are no users of banners.js using video; it is the right move. the otehr option is to do a major bump due to your previous change
3 participants
Lets the user customize the styling of the banner using the following css variables:
--ts-banner-width
--ts-banner-height
--ts-banner-padding
--ts-banner-margin
Also removes the width and height properties from the topsort-banner element.