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

[modals] Add tooltip placement bottom (articles and modules) #10076

Merged
merged 2 commits into from
May 1, 2016

Conversation

cyrezdev
Copy link
Contributor

@cyrezdev cyrezdev commented Apr 24, 2016

Minor redo #10013 .
Missing tooltip placement bottom, when this modal view is loaded in association modal.
Minor changes related only to modules and articles modals

Summary of Changes

  • Add tooltip placement bottom
  • Minor change for container div (for consistency with other modal reviews)

Testing Instructions (Multilanguages is enabled)

Articles modal

  • Go to Contents > Edit an article > Associations and select article.
  • In editor, control the TinyMCE modal, on click on the editor button "Articles"
  • In editor, control the TinyMCE modal, on click on the editor button "Modules" (this modal view is not yet used anywhere else in core)

Note: Hathor overrides to be updated later in a PR i will do with all the modals updated the last days ;-)

@cyrezdev cyrezdev changed the title Add tooltip placement bottom [modals] Add tooltip placement bottom (articles and modules) Apr 24, 2016
@cyrezdev cyrezdev changed the title [modals] Add tooltip placement bottom (articles and modules) [modal] Add tooltip placement bottom (articles and modules) Apr 24, 2016
@andrepereiradasilva
Copy link
Contributor

andrepereiradasilva commented Apr 24, 2016

I have tested this item ✅ successfully on 6f49e9c

BTW you also have the articles modal in the menu item Aingle article type "Select" button.
And on TinyMCE on frontend edit (Articles and Modal).


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

@brianteeman brianteeman changed the title [modal] Add tooltip placement bottom (articles and modules) [modals] Add tooltip placement bottom (articles and modules) Apr 24, 2016
@cyrezdev
Copy link
Contributor Author

cyrezdev commented Apr 24, 2016

BTW you also have the articles modal in the menu item Aingle article type "Select" button.
And on TinyMCE on frontend edit (Articles and Modal).

Yes, but not listed all the places where articles modal is used, as the changes are really minor, and if working in one "redo" bs modal, it should work too in other places (loading the same code! ;-) )

Thank you for testing! 👍
(i think BS modals will be really improved for 3.5.2, with a lot of code clean and more consistency ;-) )

@andrepereiradasilva
Copy link
Contributor

(i think BS modals will be really improved for 3.5.2, with a lot of code clean and more consistency ;-) )

Clearly. great job!

@cyrezdev
Copy link
Contributor Author

cyrezdev commented Apr 24, 2016

Clearly. great job!

You did a big part of it! Thank you for testing all those PRs! ;-)

@Twincarb
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 6f49e9c

Functionality in desktop is correct, however I noticed an issue when working in a mobile browser, I think it could probably be corrected with a change to the template css.
But thinking about it more, the modal in the scenario I tested doesn't work well on a mobile at all. I will raise a separate issue for that, if the pop-down can be adjusted for this PR it will at least mean it's all readable.

Apply PR(10147)
from the front end add new article, then select +article, the pop up/pop down

(I haven't tested the code below but it may fix the issue)

@media {max-width 375px){
.tooltip.fade.bottom.in{
width:300px;
}
}


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

@cyrezdev
Copy link
Contributor Author

cyrezdev commented May 1, 2016

@Twincarb Your unsuccessfull test is not related to this PR, nor the other PR you mentionned.
There's no fix here for TinyMCE modal (not Bootstrap modals, which is the purpose of my PR).

The purpose of this PR is only a tooltip position fix.
The TinyMCE modals were already not responsive before this PR and in 3.5.1, and this is not the issue i'm fixing here. ;-)
There's other improvements i'm working on for modals, but not yet for TinyMCE... And don't know yet if it's a TinyMCE library issue (external library) or if this could or should be fixed inside Joomla...

So, about the current PR, your test is wrong as not related to the issue fixed: tooltip position and padding of content (very minor changes here for the content of the modal, and not the TinyMCE modal.
Thanks to read again my first message ;-)

@cyrezdev
Copy link
Contributor Author

cyrezdev commented May 1, 2016

@Twincarb Again me!
So i realize now that your last comment here is not updated about the issue you reported, after reading here #10147 (comment) your comment. ;-)

First, could you confirm you test on last staging, and if you have clean your cache on your mobile too ?
As tooltip position should be bottom now in all screen devices, and not truncated (it was top before and truncated top).

Could you tell me which mobile OS and browser, and attach a screenshot ?
As i don't see how an issue possible about tooltip, as with this PR, the placement is bottom, and the tooltip are loaded inside the iframe, included in the modal-body...
So curious about the screenshot, and the result you have ?...

Thank you for your contribution 👍

@Twincarb
Copy link
Contributor

Twincarb commented May 1, 2016

@JoomliC
Staging is latest 30th May, Tested on Android 6.0 with latest Chrome browser cache cleaned as well.
The position of the tooltip is correct below the search box as can be seen on the screenshot below.

I have also tested it on an ipod touch and have the identical results, the issue itself is with TinyMCE, I have now finished having a good read through the TinyMCE Github Issues page, it appears there is a plan for a seperate lightweight version for mobile support, I guess we will have to live with the non mobile friendly modals we have.

I will change my test to positive in light of the above. 😄

Android View


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

@Twincarb
Copy link
Contributor

Twincarb commented May 1, 2016

I have tested this item ✅ successfully on 6f49e9c

Sorry for the HUGE picture above!

PR works as expected.


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

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 6f49e9c


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

@brianteeman
Copy link
Contributor

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 1, 2016
@brianteeman brianteeman added this to the Joomla 3.5.2 milestone May 1, 2016
@rdeutz rdeutz merged commit c95126f into joomla:staging May 1, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 1, 2016
@rdeutz rdeutz modified the milestones: Joomla 3.5.2, Joomla! 3.6.0 May 1, 2016
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

6 participants