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

Compact image modal layout #22475

Merged
merged 9 commits into from Jun 15, 2019

Conversation

Projects
None yet
10 participants
@ciar4n
Copy link
Member

commented Oct 3, 2018

Pull Request for Issue # .

Summary of Changes

Compacts the media field modal layout and increases modal size. Removes the double scroll bar on larger screens. Currently, there are 3 options to close. This PR removes the close button in the footer (admin).

Testing Instructions

Apply PR and open administration media modal (media field -> select).

Before

image

After

image

Documentation Changes Required

<div class="span4 control-group">
<div class="pull-right">
<button class="btn btn-success button-save-selected" type="button" <?php if (!empty($onClick)) :
// This is for Mootools compatibility ?>onclick="<?php echo $onClick; ?>"<?php endif; ?> data-dismiss="modal"><?php echo JText::_('COM_MEDIA_INSERT'); ?></button>

This comment has been minimized.

Copy link
@Quy

Quy Oct 3, 2018

Contributor

Indent to match below?

@Quy

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2018

Hopefully, you can come up with a fix for this related issue #17551.

@ciar4n

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2018

Hopefully, you can come up with a fix for this related issue #17551.

I will have a look. Better suited for a separate PR as it will likely touch all tiny mce modals.

@Quy

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

Toolbar is no longer pinned with this PR.

@ciar4n

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2018

Not sure if I understand. Would you have a screenshot?

@Quy

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

With this PR, go to Content > Articles, scroll down and the toolbar goes off screen and no longer is pinned.

toolbar

@ciar4n

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2018

Strangely I am unable to replicate this issue. Toolbar is sticking in both Firefox and Chrome.

@Quy

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

Using your branch, it is not an issue. However, with beta4 and patchtester, then it is reproducible.

@mbabker

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

An issue because of patchtester doesn't mean the patch has issues. It could very well be there is another change after beta 4 that patchtester isn't applying which is causing what you're seeing.

It is literally impossible to make patchtester behave the same as if you're running git apply PR.diff because the CMS environment does not have the git environment data necessary to generate the appropriate diffs.

@Quy

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

I have tested this item successfully on ed85e70


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

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

It's better but I still have a small scroll (unlike your screenshot)

Tested on Chrome 70 Windows
image

ciar4n added some commits Jan 17, 2019

@ciar4n

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

@brianteeman Should be ok now.

@Quy

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

I have tested this item successfully on c2d7df2


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

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2019

Tested again with
Firefox 64.0.2 (64-bit) Windows
Version 71.0.3578.98 (Official Build) (64-bit) Windows

I still have the same results as in #22475 (comment)

@franz-wohlkoenig franz-wohlkoenig changed the title [isis] Compact image modal layout Compact image modal layout Apr 19, 2019

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

I have tested this item successfully on 2f7270c

System information

  • 3.9, latest nightly build
  • Template: Protostar
  • macOS High Sierra, 10.13.6
  • Firefox 66 (64-bit)

CloudAccess.net

  • PHP 7.1.27
  • MySQLi 5.7.24-cll-lve

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22475.
@viocassel

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

I have tested this item successfully on 2f7270c


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

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented May 19, 2019

Status "Ready To Commit".

@roland-d

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

I don't think this can be RTC as @brianteeman is still seeing an issue

@franz-wohlkoenig franz-wohlkoenig added J3 Issue and removed RTC labels May 19, 2019

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented May 19, 2019

@roland-d thanks, had only looked at number of tests.

Status back on "Pending".

@Quy

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

@brianteeman Can you please test again?

Tested fine on Windows 10:
Firefox 67.0.2
Chrome 75.0.3770.80

@Quy

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@roland-d We have 3 successful tests. I can't reproduce the issue Brian is having. Can we please merge it anyway? It is better than before and deal with the issue Brian has in a later PR if more people report it.

@roland-d

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@HLeithner As release leader, I will let you decide.

@HLeithner HLeithner merged commit 6c6824f into joomla:staging Jun 15, 2019

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Hound No violations found. Woof!
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@HLeithner

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

It works for me fine in ff and vivaldi. I have have no idea why brian has this problem but I think its better to merge it then having the current state.

thx

@HLeithner HLeithner added this to the Joomla 3.9.9 milestone Jun 15, 2019

@ciar4n

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

Thanks all for the tests

@ciar4n ciar4n deleted the ciar4n:image-modal branch Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.