-
Notifications
You must be signed in to change notification settings - Fork 9
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
EMULSIF-304: Banner component. #152
base: convert-components-to-sdc
Are you sure you want to change the base?
Conversation
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.
@robherba This PR matches the design but I have some feedback for you:
-
Lets try and avoid committing images (png, gif, jpg) directly to the repository and use a free service instead. https://picsum.photos/ is what we've used for Compound and seemed to work well. Here is an example of how we crafted a URL which defines the image size. I don't think you need to make this a responsive image so a standard
<img src />
should be fine: https://github.com/emulsify-ds/compound/blob/main/components/02-molecules/text-with-media/text-with-media.yml#L4C16-L4C37 -
See code comments on
banner.twig
-
Lets combine the
bem()
andadd_attributes()
functions. For example:
{% set banner__attributes = {
'data-banner-alignment': banner__alignment|default('left'),
'class': bem(banner__base_class, banner__modifiers, banner__blockname)
} %}
<div {{ add_attributes(banner__attributes) }}> ... </div>
- Avoid using CSS background images. Lets use the image component instead. Pull it it below all the banner content/text. Then use
position:absolute
,z-index
, andobject-fit
styles. This makes wiring a lot easier and allows us to use responsive images.
> | ||
{# Render the video background #} | ||
{% if banner__video %} | ||
<div {{ bem('video-container', [], banner__base_class) }}> |
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.
Instead of statically creating this video markup lets use the Video component included in the UI Kit. Here is an example of how we did this for the Smith College banner: https://github.com/smithcollege-drupal/smith-drupal/blob/develop/web/themes/custom/smith/components/02-molecules/banner/banner.twig#L128
<source src="{{ banner__video }}" type="video/mp4"> | ||
Your browser does not support the video tag. | ||
</video> | ||
<div {{ bem('video-controls', [], banner__base_class) }}> |
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 playback button should be part of the video component mentioned above. You could likely create this as a sub-component in src/components/video/playback-button
. This playback button should use the button
component and extend it. Here is an example of how I did this for Smith College but unlike Smith lets make this a sub component of Video: https://github.com/smithcollege-drupal/smith-drupal/tree/develop/web/themes/custom/smith/components/01-atoms/playback-button
* - additional_attributes: Additional HTML attributes for the banner container. | ||
*/ | ||
#} | ||
{{ attach_library('emulsify/banner') }} |
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.
Lets remove this this reference. attach_library()
is a drupal function and this component library may be used for non-drupal platforms.
Summary
Adds an banner component.
How to review this pull request
Grabacion.de.pantalla.2024-08-26.a.la.s.9.14.16.a.m.mov