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

NEXT-14691 - Add pseudo modal twig blocks #3567

Conversation

lacknere
Copy link
Contributor

@lacknere lacknere commented Feb 12, 2024

1. Why is this change necessary?

Pseudo modal template is missing many twig blocks which may be used to override/extend the template easily from themes/plugins.

2. What does this change do, exactly?

Add the missing twig blocks and deprecate an inappropriately named block.

3. Describe each step to reproduce the issue or behaviour.

4. Please link to the relevant issues (if any).

NEXT-14691

5. Checklist

  • I have rebased my changes to remove merge conflicts
  • I have written tests and verified that they fail without my change
  • I have created a changelog file with all necessary information about my changes
  • I have written or adjusted the documentation according to my changes
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfil them.

Copy link

Warnings
⚠️ You probably moved or deleted a twig block. This is likely a hard break. Please check your template changes and make sure that deleted blocks are already deprecated.
If you are sure everything is fine with your changes, you can resolve this warning.
Moved or deleted block:
Array ( [0] => Array ( [0] => product_detail_zoom_modal_close_button_content [1] => component_pseudo_modal_back_btn )
)

data-bs-dismiss="modal"
aria-label="Close">
{# @deprecated tag:v6.7.0 - Block will be removed. Use `component_pseudo_modal_close_btn_content` instead. #}
{% block product_detail_zoom_modal_close_button_content %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the new block component_pseudo_modal_close_btn_content is missing :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, it's added now 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Very good :-)

@lacknere lacknere force-pushed the next-14691/add-pseudo-modal-twig-blocks branch 2 times, most recently from 577e246 to 3f9f13e Compare February 21, 2024 20:12
@tobiasberge
Copy link
Contributor

Hi @lacknere,

thank you for your contribution 🎉 and sorry for my late review.

I know these are "just" blocks, but do you really need blocks on every single element?

The good thing about the pseudo-modal right now is, that the general modal markup could change in the background (for example due to a Bootstrap update) and it would not be an issue because new PseudoModalUtil() would still work the same. If every chunk of HTML has a block, every change/moving of the HTML can be considered as a breaking change. Maybe blocks for "overall wrapper", "header", "title" and "content" would be enough.

@lacknere lacknere force-pushed the next-14691/add-pseudo-modal-twig-blocks branch from 3f9f13e to bb48ee4 Compare March 18, 2024 20:21
@lacknere
Copy link
Contributor Author

@tobiasberge I've removed the modal dialog and content blocks. The new blocks are now "overall", "header", "title", "close button", "body" and "back button". Is that okay?

@tobiasberge
Copy link
Contributor

Hi @lacknere thank you. 👍 LGTM.

And good catch with the wrong block naming product_detail_. 🚀

@shopware-github-importer
Copy link

Hello,

thank you for creating this pull request.
I have opened an issue on our Issue Tracker for you. See the issue link: https://issues.shopware.com/issues/NEXT-34491

Please use this issue to track the state of your pull request.

@tobiasberge
Copy link
Contributor

Hi @lacknere, you changes have been merged to trunk: f5bc948 and will also be backported to 6.5.x

Thank you for your contribution. 🎉

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

4 participants