Skip to content
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

feat(banner): add coypright - FRONT-3616 #2404

Merged
merged 14 commits into from
May 6, 2022
Merged

Conversation

emeryro
Copy link
Contributor

@emeryro emeryro commented Apr 28, 2022

Add optional copyright to banners and carousel

@emeryro emeryro self-assigned this Apr 28, 2022
@github-actions
Copy link

github-actions bot commented Apr 28, 2022

1 similar comment
@github-actions
Copy link

@@ -36,6 +36,23 @@ $_font-weight: null !default;
position: relative;
}

.ecl-hero-banner__copyright {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's true that we only use this twice, but couldn't we make a mixin and include it where needed, instead of duplicating these rules..?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are already duplicating all the other rules for the banners.
We could have some generic banner css, and just change what correspond to page or hero banner. But I would keep this for another ticket

if (data.image) {
argTypes.show_copyright = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this control seems uneffective, you don't use it to control the display of the copyright.

@@ -58,6 +60,9 @@
<section class="{{ _css_class }}"{{ _extra_attributes|raw }}>
{% if _image is not empty and _variant in ['image', 'image-shade', 'image-gradient'] %}
<div class="ecl-page-banner__image" style="background-image:url({{ _image }})"></div>
{% endif %}
{% if _copyright is not empty and _variant in ['image', 'image-shade', 'image-gradient'] %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is the right approach. i mean that i would imagine a use case where the copyright is needed but no text would be associated to it, not sure if it's a valid use case but in this case we should probably have a boolean to control the display and a text parameter to set an optional text, but you know what the requirements are better than me..

@planctus planctus merged commit 5303610 into v3-dev May 6, 2022
@planctus planctus deleted the FRONT-3616-copyright branch May 6, 2022 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants