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

Bootstrap popup with mootols compatibility #10788

Closed
wants to merge 1 commit into from
Closed

Bootstrap popup with mootols compatibility #10788

wants to merge 1 commit into from

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Jun 11, 2016

Joomla 3.5 should had bootstrap modal for form field media but didn't. This fixes that

Summary of Changes

Introduce an option that will provide the needed backwards compatibility.

Testing Instructions

Apply patch

  • Bootstrap mode
    Test creating a new article and that the intro/full text images as well as the tinymce image button still works.
  • Mixed mode
    Edit administrator/components/com_content/views/article/tmpl/edit.php
    and paste the code bellow after line 84 (just before ?>)
JHtml::_('behavior.modal');

$idTag = 'jform_name';
$remoteUrl = 'index.php?option=com_media&view=images&tmpl=component&asset=com_users&author=' . JFactory::getUser()->id . '&fieldid=' . $idTag;
$buttonId = $idTag . '_btn';

JHtml::_('bootstrap.modal');
$test_control[] = '<a id="' . $buttonId . '" href="#' . $idTag . '_modal" role="button" class="btn" data-toggle="modal" title="' . JText::_('JSELECT') . '">SELECT CUSTOM BS</a>';
$test_control[] = JHtmlBootstrap::renderModal(
    $idTag . '_modal',
    array(
        'url' => $remoteUrl,
        'title' => JText::_('JSELECT'),
        'height' => '600px', 'width' => '500px')
);

$test_control[] = '<input type="text" title="cscsc" id="jform_name" >';

$test_control[] = '<a class="modal btn" title="XXXXX" href="index.php?option=com_media&amp;view=images&amp;tmpl=component&amp;asset=image&amp;author='
    . JFactory::getUser()->id . '&amp;fieldid=jform_name&amp;folder="'
    . ' rel="{handler: \'iframe\', size: {x: 800, y: 500}}">SELECT MOOTOOLS</a>';

echo implode('', $test_control);

//JFactory::getDocument()->addScriptDeclaration("
//    window.jInsertFieldValue = function(value, id) {jQuery('#' + id).val(value);};
//    ");

Test again the previous scenario as well as the new buttons that will appear above the tabs. The field should be field with the image selected in the pop up

  • Compatibility Mode (AKA Mootools)
    Keep the changes in edit.php
    Rename administrator/templates/isis/html/layouts/joomla/form/field/media.php to xxxmedia.php
    Repeat the previous testing steps

@cyrezdev
Copy link
Contributor

@dgt41 Clearly, i don't like the idea of adding an override in Isis template, as this does not allow user to create its own layout override...
This why i did the PR #10772 with a new form field type : modal_user

But your idea of checking if is mootools is a nive approach.
This, with no override, but as a new form field type, could be better, no ? And achieve the same goal, without having to override the layout.
Clearly, i think we are near the objective : use BS modal. But should go with no override to not introduce a new issue ;-)

Note: in my PR, i've added the new basetype attribute, but the field type could be modal_media if basetype is not mentionned, it will follow the same logic as type="media" and basetype="modal"

@ghost
Copy link

ghost commented Jun 11, 2016

I have tested this item ✅ successfully on 2a1b1e4


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

@dgrammatiko
Copy link
Contributor Author

@JoomliC I think JLayouts overrides are PART of a template, which means you cannot change them (without creating a new template). So, in that sense, templates CAN and SHOULD have their own JLayouts if the required output is different than the standard (/root/layouts).
This PR just enables what should have been the option in 3.5: bootstrap modal for the field media.
I have a similar solution for user field (again with a $ismoo switch, defaulting to mootools) that I am currently testing and will PR soon.
There are great things in your PR but WE NEED to provide a WORKING repeatable, not a broken like the old one, these changes ensure that!
About the idea of using AJAX in repeatable: I think will not solve the problem as the fields should be able to be manipulated in the client side and depending on id's is just makes things too complicated. The idea of using wrapper/container that @Fedik is using is solid, but comes with the cost that fields with bootstrap modals need a container and some kinda initialisation script. Said that the only field that won't work with the new repeatable is calendar, but I have a solution for that (maybe 3.6.1 as I am still too busy with a project)

@cyrezdev
Copy link
Contributor

cyrezdev commented Jun 11, 2016

SHOULD have their own JLayouts if the required output is different than the standard (/root/layouts).

@dgt41 I agree, and that's why the standard (BS modal) should not be an override ;-)
That's where using a new form type could fix this issue, and change nothing to your code. (just move to a subfolder "modal" and use basetype "modal_" for media type...
Or standard is still mootools modal and i'm stupid? ;-)

Could you check again if basetype could help or not ? (if not, i will stop working on this...)

In the same time, repeatable subform script is not working, as issue with field incrementation: #10772 (comment)

@mbabker
Copy link
Contributor

mbabker commented Jun 11, 2016

Or standard is still mootools modal ?

If you can make major changes to fields without breaking compatibility in their APIs, you can change the stuff. Considering that SqueezeBox and Bootstrap modals don't have compatible JavaScript APIs, that makes anything at least from that perspective difficult to deal with. But what was found in 3.5 testing (IIRC) was that there isn't a clean way to migrate some of these fields from 100% MooTools framework to 100% jQuery/Bootstrap framework without some kind of deep rooted B/C break. So really the only options in that case are either complex configuration directives, new field types, or waiting for 4.0.

@dgrammatiko
Copy link
Contributor Author

@mbabker with this PR all cases are covered:

  • Using Old mootools layout field
  • Using new Bootstrap field but with fallback for 3PD that are using the javascript of the old field (dynamically created field, XML will be rendered with the new layout)
    So I thing this is covering all bases, unless I am missing something.

@cyrezdev
Copy link
Contributor

@mbabker This is why i proposed a new field type build by a new field attribute basetype to keep B/C for third party extension too : https://github.com/joomla/joomla-cms/pull/10772/files#diff-ed54af5d39d21ed9a84d28eba903bd5fR1779

@cyrezdev
Copy link
Contributor

@dgt41 Well, now my mind is better opened ( ;-) ) i will test your PR tomorrow if enough time (children at home this week-end)
And your planned PR for user form field won't be a dupplicate of mine, as i will see this one now as an "extended" user field ;-)

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Jun 11, 2016

@anibalsanchez @ggppdk can you test this one? It should be fixing #9454 and still providing bootstrapped media field! Thanks

@dgrammatiko
Copy link
Contributor Author

@Fedik can you review this one?

@anibalsanchez
Copy link
Contributor

Hi,

Test instructuions for Mixed mode does not match the code in edit.php. After line 74, there is no immediate ?> and there is a bunch of code until the next ?>.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Jun 11, 2016

@anibalsanchez yup my staging was behind, should be after line 84
EDIT
just before the first div

@Fedik
Copy link
Member

Fedik commented Jun 12, 2016

I have tested this item ✅ successfully on 2a1b1e4


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

@cyrezdev
Copy link
Contributor

I have tested this item ✅ successfully on 2a1b1e4

A reason for not added the tab for upload file as you did in 3.5 RC ? (it was a nice idea ;-) )


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

@brianteeman
Copy link
Contributor

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 13, 2016
@dgrammatiko
Copy link
Contributor Author

@JoomliC

A reason for not added the tab for upload file as you did in 3.5 RC ? (it was a nice idea ;-) )

There was a discussion few months ago and due to the B/C break or the addition of an extra html override the decision was to delay this change till J4...
I prefer as well that one over the long form, but I am no the one deciding here 😀

@cyrezdev
Copy link
Contributor

@dgt41 Thanks for info! 👍
The same preference for me 💯

@dgrammatiko
Copy link
Contributor Author

Replaced by #10889 Thanks everybody for testing here!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 28, 2016
@zero-24
Copy link
Contributor

zero-24 commented Jun 28, 2016

@brianteeman @wilsonge please remove the milestone here as it get closed without merging as it is moved to a different pull request!

@dgrammatiko dgrammatiko deleted the resoreImageBSModals branch October 11, 2016 11:34
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

9 participants