Skip to content

Conversation

@bahl24
Copy link
Contributor

@bahl24 bahl24 commented Feb 14, 2019

Pull Request for Issue - When viewing information about an image the text overlaps when toggle menu is open both in PC as well as mobile devices.
Mobile
screenshot from 2019-02-14 16-41-25
PC
screenshot from 2019-02-14 16-41-08

Summary of Changes

Removed flex property which solves the issue both in PC and Mobile irrespective of whether toggle menu is open or closed
Mobile-
screenshot from 2019-02-14 16-40-11
PC-
screenshot from 2019-02-14 16-39-09

Testing Instructions

Components->media->select an image->info

Documentation Changes Required

No

Signed-off-by: Nitish Bahl <[email protected]>
@bahl24
Copy link
Contributor Author

bahl24 commented Feb 14, 2019

@infograf768 @dgrammatiko @Quy @C-Lodder Kindly provide feedback regarding the design changes

@C-Lodder
Copy link
Member

all seems fine to me

@joomla-cms-bot joomla-cms-bot added the NPM Resource Changed This Pull Request can't be tested by Patchtester label Feb 21, 2019
@bahl24
Copy link
Contributor Author

bahl24 commented Feb 21, 2019

@infograf768 @Quy Kindly test this

@infograf768
Copy link
Member

This does not solve RTL.
screen shot 2019-02-21 at 08 59 16

(Would need bootstrap override specific to RTL)

In my patch for RTL I have used ul/li and no flex to solve all issues, including in LTR.

RTL
screen shot 2019-02-21 at 09 18 16

LTR
screen shot 2019-02-21 at 09 18 48

Mobile
As you can see, I have also modified css to get more space for the name of the file in mobile vue

screen shot 2019-02-21 at 09 25 36

I have not proposed this patch yet as it waits for other PRs to be merged, specially
#23950
and
#23756

@bahl24
Copy link
Contributor Author

bahl24 commented Feb 21, 2019 via email

@infograf768
Copy link
Member

Please don't close.
the decision to use ul/li instead of ol/etc. should be decided by maintainers.
@C-Lodder @wilsonge
I could update my future patch to your proposal with a bootstrap override and my other modififications should still be possible.

@C-Lodder
Copy link
Member

C-Lodder commented Feb 21, 2019

@infograf768 ol vs ul don't need a decision by maintainers. One is an ordered list (1, 2, 3, etc) and the other isn't. In general, using a list would probably be better as there shouldn't be any issues with RTL

@infograf768
Copy link
Member

In general, using a list would probably be better as the RTL should kick in automatically.

I do prefer ul in general as they are easier to cope with and we do not need flex at all in this case.
Still needs some tweaks for RTL concerning com_media.

Look here the non-minified css file obtained after my patch. At the bottom of the file are all the RTL overrides, including for infovue.
mediamanager.min.css.zip

@infograf768
Copy link
Member

infograf768 commented Feb 26, 2019

OK, I am told that it is better to use <dl> etc.
Will try to deal with rtl, using your patch.

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 7cbfa13

OK for LTR. RTL is another story to be dealt with separately. Thanks.

One more tester please.


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

@infograf768
Copy link
Member

@C-Lodder
Please confirm test.

@Razzo1987
Copy link
Contributor

For me doesn't work :(

screen shot 2019-02-26 at 09 17 23


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

@infograf768
Copy link
Member

@Razzo1987
Have you ran npm ci on the test site?
If not, then the patch is not enough...

@Razzo1987
Copy link
Contributor

Sotty, I didn't.

I understand now I miss this step on my tests. Now I'm redone it


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

@wilsonge wilsonge merged commit b2e6d01 into joomla:4.0-dev Feb 26, 2019
@wilsonge wilsonge added this to the Joomla 4.0 milestone Feb 26, 2019
@wilsonge
Copy link
Contributor

Merged on JM's request. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants