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(modal): add modal component - FRONT-3858 #2776

Merged
merged 29 commits into from
Mar 9, 2023
Merged

Conversation

emeryro
Copy link
Contributor

@emeryro emeryro commented Feb 20, 2023

No description provided.

@emeryro emeryro changed the base branch from v3-dev to v3.8.0-dev February 20, 2023 12:52
@github-actions
Copy link

github-actions bot commented Feb 21, 2023

Copy link
Contributor

@planctus planctus left a comment

Choose a reason for hiding this comment

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

Some minor things and a question regarding the overall approach in the definition of this component.

src/implementations/twig/components/modal/modal.html.twig Outdated Show resolved Hide resolved
src/implementations/twig/components/modal/modal.html.twig Outdated Show resolved Hide resolved
src/implementations/vanilla/components/modal/_modal.scss Outdated Show resolved Hide resolved
src/implementations/vanilla/components/modal/_modal.scss Outdated Show resolved Hide resolved
src/implementations/vanilla/components/modal/_modal.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@planctus planctus left a comment

Choose a reason for hiding this comment

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

Looking good, only small remarks ;)

src/implementations/twig/components/modal/modal.html.twig Outdated Show resolved Hide resolved
@use 'sass:map';
@use '@ecl/theme-dev/theme';

.ecl-modal {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now it looks much better, but there is no margin between the two buttons:
image

This only happens when enabling also the "default-print" css

{%- block header %}{{ _header }}{% endblock -%}
</div>
{% endif %}

Copy link
Contributor

Choose a reason for hiding this comment

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

also here i would add a check for the icon_path not being empty to determine whether to pass also the icon or just the button.

@emeryro
Copy link
Contributor Author

emeryro commented Mar 9, 2023

For the buttons, I'm not sure that it is a real case. The ecl-print-default css is not supposed to be used when not printing (so with the "normal" ECL css), and if you have the ecl-print css it is displayed correctly (even with the ecl-print-default)

@planctus planctus merged commit 1055530 into v3.8.0-dev Mar 9, 2023
@planctus planctus deleted the FRONT-3858-modal branch March 9, 2023 13:35
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.

None yet

2 participants