? Success

User tests: Successful: Unsuccessful:

avatar JoomliC
JoomliC
26 Apr 2016

Improve the PR #9908 (remove modal html from default view, to create body and footer layouts for each modals, and to load each modal with its unique DOM identifier).

Summary of Changes

  • Improve #10071, modalTooltip container: change done in modal layout contructor (main) to allow to set container to the modal ID (selector) without having to add this each time in each body content.
  • Rename "collapseModal" id into "copyModal" : consistency with all other significant modal naming.
  • Fix select File Type if no file extension selected (no client side validation, as empty value was plain text "null", replace by "")

Layouts change:

  • Copy template (copyModal), Rename File (renameModal) and Resize Image (resizeModal) : add div group-label, unique div id, replace hasTooltip by modalTooltip (will display previously hidden tooltips).
  • New File (fileModal) & Manage Folders (folderModal) : clarify BS HTML forms, and add a few improvements: scrolling inside list of files/folders (column left), fixed position top for form fields (column left), media queries for form fields at top modal on small devices...

Testing Instructions

  • Go to Extensions > templates > templates
  • Open one template to go to template manager
  • Test following buttons and related modals: Copy template (collapseModal) Rename File (renameModal) Delete File (deleteModal) New File (fileModal) Manage Folders (folderModal) Resize Image (resizeModal)

Example of "New File" modal with the new scrolling in list of folders and new display for form fields:

Before Patch :
Form fields to create a file disappear from view on scroll.
capture d ecran 2016-04-27 a 01 42 05

After Patch :
Form don't move with scrolling (stay at top), inline form for new file name...
capture d ecran 2016-04-27 a 01 42 30

avatar JoomliC JoomliC - open - 26 Apr 2016
avatar JoomliC JoomliC - change - 26 Apr 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 26 Apr 2016
Labels Added: ?
avatar JoomliC JoomliC - change - 27 Apr 2016
The description was changed
avatar brianteeman brianteeman - change - 27 Apr 2016
Category Components UI/UX
avatar brianteeman brianteeman - test_item - 27 Apr 2016 - Tested successfully
avatar brianteeman
brianteeman - comment - 27 Apr 2016

I have tested this item :white_check_mark: successfully on c8cd1f9


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

avatar infograf768
infograf768 - comment - 27 Apr 2016

hmm
I get again:
screen shot 2016-04-27 at 11 48 25

Also, you are deleting 2 files and you do not modify script.php (forget if the files were added in 3.5.2)

avatar JoomliC
JoomliC - comment - 27 Apr 2016

Also, you are deleting 2 files and you do not modify script.php (forget if the files were added in 3.5.2)

Yes, the files were created and merged in 3.5.2-dev ;-)
In the first PR, i just created the modals body and footer as layouts, and used the existed names for modal identifier.
I did here the change for consistency, and have a logical-related name for identifier of each modal (it was already good for others, only copy modal was identified as "collapseModal" which is in fact used as identifier only for batch modals today (maybe another PR to replace all collapseModal by batchModal when used as so...)

I get again:

About tooltip, what is exactly the problem ?
What i did was to use the same BS HTML strucuture as used in all Joomla forms.
If you do as Batch (not good in my opinion), you will have the tooltip opened and centered on all the div width, not on the label.
This gives a strange tooltip placement in batch modals (and the more your screen is wide, the more it's weird) :
capture d ecran 2016-04-27 a 14 39 57
Example with Batch Modal, with tooltip not placed on top of the label.

In fact, this change is to have exactly the same rendering as the renderField layout : https://github.com/joomla/joomla-cms/blob/staging/layouts/joomla/form/renderfield.php
We can use directly the layout here (maybe a not too bad idea?, as this way the display is control by the layout renderField...) to render the field:

JLayoutHelper::render('joomla.form.renderfield', array('input' => $input, 'label' => $label, 'options' => ''));

Indead, this layout renderfield is not really used today in view, but maybe because views were created before this layout... (@phproberto may help in decision here with his advice, if switch to the joomla.form.renderfield or to keep full html structure inside the view here, as it is now ? ;-) )

In Batch modal, the fields are not rendered the good way, and the span6 was not used for the tooltip on label, but for the fields in 2 columns (another PR i will think to do, to use the renderField layout for each field of batch, as each one are already layout fields).

If you mean the fact the tooltip go out the modal (left), unfortunately, we face here the limitation of the tooltip with no auto placement depending on the container. (i think it's improved in last version of bootstrap, and a bit better in BS3, but BS2 is limited)

avatar grhcj grhcj - test_item - 27 Apr 2016 - Tested successfully
avatar grhcj
grhcj - comment - 27 Apr 2016

I have tested this item :white_check_mark: successfully on c8cd1f9


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

avatar infograf768
infograf768 - comment - 27 Apr 2016

@JoomliC

If you mean the fact the tooltip go out the modal (left), unfortunately, we face here the limitation of the tooltip with no auto placement depending on the container. (i think it's improved in last version of bootstrap, and a bit better in BS3, but BS2 is limited)

Yes I mean this. I know why the span6 was used in the batch modals. Until we get BS2 or whatever version, adding the span6 for this specific modal looked like the only solution to make this tip readable.

But, if one reduces the screen, we get it nice and clear:

screen shot 2016-04-27 at 18 03 28

Would be great to get such a result on a monitor.

avatar JoomliC
JoomliC - comment - 28 Apr 2016

@infograf768 What is missing is top-left, top-right, bottom-left, bottom-right... position in Boostrap 2 tooltip.
The fact is in current bootstrap.js, tooltip in top and bottom placement are always centered on the container.
So, in this PR, the placement of the tooltip on the label (and not on a grid span6 element) is the good one (and it's still bad way for batch modals, as not set to label...).

But i agree definitely that a top-left placement (top-right on RTL) for tooltip when inside a modal would be the real solution for this, but not related directly to this PR.

So, i think this PR, could be RTC if you think everything else is ok (as already 2 successfull tests) and in all cases the result is better than in 3.5.1 (where tooltip was behind the modal!).

About a top-left placement when a .modalTooltip is used, this could be achieve in 2 ways:

  • using a tooltip-extension for bootstrap (or to adapt BS3 tooltip), but this suppose to edit the js library (was already partially changed for Joomla... and i think this could be edited for tooltip in a B/C way, with nothing change excepted the possibility to have extra-placements).
  • Or adding in modal main layout, (layouts/joomla/modal/main.php) the following lines for the top-left placement (to be adapted to RTL too, not difficult) EDIT: not a good idea... Really better this: #10100 (comment) :
    $script[] = "       $('.modalTooltip').mouseover(function(){";
    $script[] = "           var left = parseInt($('.tooltip').css('left'));";
    $script[] = "           var labelWidth = parseInt($('.modalTooltip').outerWidth(true));";
    $script[] = "           var leftTop = left - (left-labelWidth);";
    $script[] = "           if ((left-labelWidth) > 0){";
    $script[] = "               $('.tooltip').css('left', leftTop + 'px');";
    $script[] = "           } else {";
    $script[] = "               $('.tooltip').css('left', '0');";
    $script[] = "           }";
    $script[] = "           $('.tooltip-arrow').css('left', '25px');";
    $script[] = "       });";

This code (to be added for quick test, just before the last $script[] line (not in bs.modal functions) is just a first attempt i've done this evening (so, not perfect...), and you will see that changing the file this way, it will works just now in this PR as well as in all batch modals ;-)

But concerning this PR, the goal was a better HTML rendering, and consistency about modal identifiers.
And as for this reason, i have renamed the 2 files for copyModal, could be good too to add the 3.5.2 label, to not have to edit script to remove files already merged in 3.5.2-dev ;-)

Thanks!

avatar JoomliC
JoomliC - comment - 28 Apr 2016

@infograf768 i can create a PR to integrate this tooltip extension for bs2 which works like a charm : https://github.com/andresgutgon/bootstrap-tooltip-extension/blob/master/README.md
And then, add only a minor change to modal layout, to allow a top-left position in all modals ;-)

Now, the question would be if this could be accepted by PLT to add this tooltip extension for additionnal placements... (note: this is full B/C ;-) )

avatar Kubik-Rubik Kubik-Rubik - change - 29 Apr 2016
Status Pending Ready to Commit
avatar Kubik-Rubik Kubik-Rubik - change - 29 Apr 2016
Milestone Added:
avatar Kubik-Rubik
Kubik-Rubik - comment - 29 Apr 2016

Looks good, thank you @JoomliC. I would also like to see your improvement for the tooltips in another PR! :-)

avatar joomla-cms-bot joomla-cms-bot - change - 29 Apr 2016
Labels Added: ?
avatar Kubik-Rubik Kubik-Rubik - change - 29 Apr 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-04-29 11:01:42
Closed_By Kubik-Rubik
avatar Kubik-Rubik Kubik-Rubik - close - 29 Apr 2016
avatar Kubik-Rubik Kubik-Rubik - merge - 29 Apr 2016
avatar joomla-cms-bot joomla-cms-bot - close - 29 Apr 2016
avatar Kubik-Rubik Kubik-Rubik - reference | 87881a1 - 29 Apr 16
avatar Kubik-Rubik Kubik-Rubik - merge - 29 Apr 2016
avatar Kubik-Rubik Kubik-Rubik - close - 29 Apr 2016
avatar joomla-cms-bot joomla-cms-bot - change - 29 Apr 2016
Labels Removed: ?
avatar JoomliC
JoomliC - comment - 29 Apr 2016

Looks good, thank you @JoomliC. I would also like to see your improvement for the tooltips in another PR! :-)

Great!
So, will do this PR maybe in the afternoon (asap).
Just wonder, if i edit bootstrap.js to integrate tooltip extension, is there a recommendation on the compression tool to be used for min.js ?

avatar Kubik-Rubik
Kubik-Rubik - comment - 29 Apr 2016

Just wonder, if i edit bootstrap.js to integrate tooltip extension, is there a recommendation on the compression tool to be used for min.js ?

No, just take a tool you prefer and which does the job right! :-)

avatar rdeutz rdeutz - change - 1 May 2016
Milestone Removed:
avatar rdeutz rdeutz - change - 1 May 2016
Milestone Added:
avatar rdeutz rdeutz - change - 1 May 2016
Milestone Added:
avatar rdeutz rdeutz - change - 1 May 2016
Milestone Removed:
avatar JoomliC JoomliC - reference | 427aa47 - 4 May 16
avatar JoomliC
JoomliC - comment - 4 May 2016

@Kubik-Rubik So, in fact, i created a full BS tooltip extended plugin (working both on BS2 and BS3), as the one mentionned above was not updated, with issues, and not maintained since 3 years.
And i open just now the PR to introduce extra tooltip positions + RTL support with the auto-dir attribute : #10237

@infograf768 i think now, what you whished as tooltip placement in modal would be possible ;-)

Add a Comment

Login with GitHub to post a comment