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

Move content history to bs modal #4561

Closed
wants to merge 10 commits into from
Closed

Move content history to bs modal #4561

wants to merge 10 commits into from

Conversation

dgrammatiko
Copy link
Contributor

Same as #4513 and #4514 but for content history

Content history is currently using mootools modal.
Without affecting in anyway the logic of the field we can use jQuery and bootstrap.

B/C
Should be 100% compatible

Testing:

Go to administrator -> Banners -> create a new banner
change the alternative text and save it again
repeat the last part for few more times

press the versions button
Everything should be ok

Before:
screenshot 2014-10-12 03 17 54

After:
screenshot 2014-10-12 03 17 04

Same as #4513 and 4514 but for content history

Content history is currently using mootools modal.
Without affecting in anyway the logic of the field we can use jQuery and bootstrap.

B/C
Should be 100% compatible

Test:
Apply the patch
Go to administrator -> Banners -> create a new banner
change the alternative text and save it again
repeat the last part for few more times

press the versions button
Everything should be ok
@dgrammatiko
Copy link
Contributor Author

Proposed title:
screenshot 2014-10-12 20 07 06

@infograf768
Copy link
Member

Why:

-       JHtml::_('behavior.modal', 'a.modal_jform_contenthistory');
-
+       JHtml::_('bootstrap.modal');
+
+       $lang = JFactory::getLanguage();
+       $extension = 'com_contenthistory';
+       $base_dir = JFactory::getApplication()->isAdmin() ? JPATH_ADMINISTRATOR : JPATH_SITE;
+       $language_tag = $lang->getName();
+       $reload = true;
+       $lang->load($extension, $base_dir, $language_tag, $reload);

which is anyway wrong as you should use $lang->getTag(); instead of $lang->getName();

@infograf768
Copy link
Member

Hmm, found out that we indeed have to reload language to get the modal title translated.
Not sure we need to check the $app and else as anyway the language file is only in admin
so we only need:

$lang = JFactory::getLanguage();
$lang->load('com_contenthistory', JPATH_ADMINISTRATOR, $lang->getTag(), true);

@@ -8,9 +8,29 @@
*/

defined('_JEXEC') or die;

JFactory::getDocument()->addStyleDeclaration('
Copy link
Member

Choose a reason for hiding this comment

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

I think you will get problem with approval, with such things ... problem:

  • use the inline css, that repeats in different files
  • use !important

This make problems for support this code in future.
I think better make some 'unified' styling for the modal in the template style

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree strongly with @Fedik. Don't use inline styles and especially no !Important. I will personally redirect all complaints from template designers to you and share your home address with them 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Bakual Hmmm thats a nice way to make new customers 😃. @Fedik Actually this code is a quick hack which once we do some changes in less files won’t be needed. i pushed the changes here and at the media field and user field. Will do it for all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One more think to get 100% B/C is needed here:
I removed the moo tools modal close code SqueezeBox.close(); and replaced it with jQuery("#userModal").modal("hide”); Best practice I think is to proxy the old function to the new if SqueezeBox is not defined. Some JS guru for this? @Fedik ?

@Fedik
Copy link
Member

Fedik commented Oct 13, 2014

@dgt41 I am not guru, and for me jQuery("#userModal").modal("hide”); is good 😄

another notice:
you use JHtmlBootstrap::renderModal
but better do it in Joomla! way JHtml::_('bootstrap.renderModal', $selector, $params , $footer)
example your:

JHtmlBootstrap::renderModal('versionsModal', array( 'url' => $link, 'title' => $label ,'height' => '600px', 'width' => '800px'), '');

will become:

JHtml::_('bootstrap.renderModal',  'versionsModal', array( 'url' => $link, 'title' => $label ,'height' => '600px', 'width' => '800px'));

@trangredweb
Copy link

@test

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

@mrunalpittalia
Copy link

@tested Succesfullyscreen shot 2014-10-17 at 06 31 55

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

@mrunalpittalia
Copy link

moving to RTC as we have more that 2 successful tests

@mrunalpittalia
Copy link

moving to RTC as we have more than 2 successful tests

@dgrammatiko
Copy link
Contributor Author

The PRs regarding Media Field, User Field and Content History are also used in the front end. That might break the rendered design IF THE TEMPLATE is not BOOTSTRAP compatible. (the old modal uses it’s own css file). I wrote it!

@dgrammatiko
Copy link
Contributor Author

For B/C I reverted the option to use the mootools modal in front end. Lets NOT break every site out there.
Please test again

@dgrammatiko
Copy link
Contributor Author

Will try again when #3231 is committed

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

8 participants