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

[3.9] Move confirm consent to use the bs modal vs mootools #22926

Merged
merged 2 commits into from Nov 13, 2018

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented Nov 2, 2018

Partial Pull Request for Issue #22908

Summary of Changes

This replaces the Mootools modal vs a bootstrap modal

Testing Instructions

  • Install 3.9.0
  • enable the Content - ConfirmConsent Plugin
  • configure this plugin to point to a article
  • open a contact page on the website
  • make sure the title is clickable and opens an modal
  • apply this patch
  • make sure it is still opening a modal this time it should be a bs modal

Expected result

bs modal

Actual result

mootools modal

Documentation Changes Required

none.

Additional Information

When this here gets approved & tested I can apply similar code to the other places where we use the old mootools modal:

cc @brianteeman as he is the original author of the modal things in this plugin touched here.

@dgrammatiko
Copy link
Contributor

@zero-24 please use a layout, do not do html markup inside the plugin!

@zero-24
Copy link
Contributor Author

zero-24 commented Nov 2, 2018

Can you please let me know what you want to change? As beyond the (already hardcoded) hight and width I did not add any extensive new HTML part in that plugin right? Or is there something special you want me to change?

@dgrammatiko
Copy link
Contributor

There should be a tmpl folder with a layout (default.php or whatever) so people could actually do whatever they want in the front end, in other words, separate concerns. Check the https://github.com/joomla/joomla-cms/tree/staging/plugins/system/stats which was done correctly. This thing right now is hardcoded you cannot override anything unless you rewrite the whole plugin...

Co-Authored-By: zero-24 <zero-24@users.noreply.github.com>
@Bakual
Copy link
Contributor

Bakual commented Nov 3, 2018

While it is true that using a layout is better, this PR is valid by itself. Moving the thing to a layout can be done easily in a separate PR as well and is actually preferred. Since it makes the PRs more atomic and easier to test and review.

$modalParams['url'] = $this->getAssignedArticleUrl($privacyArticle);
$modalParams['height'] = 800;
$modalParams['width'] = "100%";
echo HTMLHelper::_('bootstrap.renderModal', 'modal-' . $modalName, $modalParams);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't output content here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zero-24 reminder, this needs to be fixed before merging. Can't output content in onContentPrepareForm().

@dgrammatiko
Copy link
Contributor

While it is true that using a layout is better

I'll say that it's mandatory, not better. You're not in Joomla 1.5 designers and site builders need to have the ability to override the output without rewriting the plugin! My 2c anyways

@mbabker
Copy link
Contributor

mbabker commented Nov 3, 2018

It should be a separate pull request for exactly the reasons @Bakual already stated. Nobody is saying not to move the code to layouts but when you start adding all sorts of extras onto pull requests they end up being too complex to properly test and review and people either ignore them or they end up getting merged while being completely broken.

@ReLater
Copy link
Contributor

ReLater commented Nov 3, 2018

I have tested this item ✅ successfully on c7ac385


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22926.

1 similar comment
@drmenzelit
Copy link
Contributor

I have tested this item ✅ successfully on c7ac385


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22926.

@Quy
Copy link
Contributor

Quy commented Nov 13, 2018

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22926.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 13, 2018
@zero-24 zero-24 added this to the Joomla 3.9.1 milestone Nov 13, 2018
@rdeutz rdeutz merged commit 9827a47 into joomla:staging Nov 13, 2018
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 13, 2018
@zero-24 zero-24 deleted the bs_modal branch April 22, 2019 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet