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

[com_templates] template manager modals (improve #9908 & #10071) #10100

Merged

Conversation

cyrezdev
Copy link
Contributor

@cyrezdev cyrezdev commented Apr 26, 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 Copy template modal missing tip #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

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on c8cd1f9


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

@infograf768
Copy link
Member

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)

@cyrezdev
Copy link
Contributor Author

cyrezdev commented Apr 27, 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)

@grhcj
Copy link

grhcj commented Apr 27, 2016

I have tested this item ✅ successfully on c8cd1f9


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

@infograf768
Copy link
Member

@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.

@cyrezdev
Copy link
Contributor Author

cyrezdev commented Apr 28, 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: [com_templates] template manager modals (improve #9908 & #10071) #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!

@cyrezdev
Copy link
Contributor Author

@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 ;-) )

@Kubik-Rubik Kubik-Rubik added this to the Joomla 3.5.2 milestone Apr 29, 2016
@Kubik-Rubik
Copy link
Member

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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 29, 2016
@Kubik-Rubik Kubik-Rubik merged commit 87881a1 into joomla:staging Apr 29, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 29, 2016
@cyrezdev
Copy link
Contributor Author

cyrezdev commented Apr 29, 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 ?

@Kubik-Rubik
Copy link
Member

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! :-)

@rdeutz rdeutz modified the milestones: Joomla 3.5.2, Joomla! 3.6.0 May 1, 2016
cyrezdev added a commit to cyrezdev/joomla-cms that referenced this pull request May 4, 2016
@cyrezdev
Copy link
Contributor Author

cyrezdev commented May 4, 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 ;-)

Kubik-Rubik pushed a commit that referenced this pull request May 8, 2016
…0237)

* Add bootstrap tooltip extension

* Revert Tooltip container, not needed anymore since #10100

* Add bootstrap-tooltip-extended.js to bootstrap.js

* Add Tooltip placement 'top-left' as default in BS modal

* Add tooltip-extended css + run generatecss

* Revert "Add tooltip-extended css + run generatecss"

This reverts commit 0a0199e.

* Revert "Add bootstrap-tooltip-extended.js to bootstrap.js"

This reverts commit 8b91262.

* Revert "Add bootstrap tooltip extension"

This reverts commit b365733.

* Create class bootstrap.tooltipExtended

This reverts commit b365733.

* Edit comment
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

7 participants