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

[4.0] Fix form grid layout #34145

Closed
wants to merge 2 commits into from
Closed

Conversation

eopws
Copy link

@eopws eopws commented May 23, 2021

Pull Request for Issue #33480 and #34081.

Summary of Changes

Added the logic of addin' horizontal scrollbar to grid form when not havin' enough width for elements

Testing Instructions

Create a menu item of "Category Blog" type and go to the "Blog Layout" tab

Actual result BEFORE applying this Pull Request

image

Expected result AFTER applying this Pull Request

img

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels May 23, 2021
@eopws eopws changed the title Add "overflow" property [4.0] Fix form grid layout May 23, 2021
@chmst
Copy link
Contributor

chmst commented May 25, 2021

Sorry, no, Horizontal scrolling is not responsive. The line must break, as it does on smaller devices.

@eopws
Copy link
Author

eopws commented May 25, 2021

Sorry, no, Horizontal scrolling is not responsive. The line must break, as it does on smaller devices.

Ok, I'm gonna fix it

@eopws
Copy link
Author

eopws commented May 25, 2021

Through few hours of searchin the only one solution that I found is to reduce count of columns, but I'm not sure if it is a good architecture solution (change layout of the whole template in order to make one page look better?)...

I would love to see a comment from a more experienced developer than me...

img

@ghost
Copy link

ghost commented Jun 4, 2021

Should this pull request get a test?

@ghost
Copy link

ghost commented Jun 8, 2021

I have tested this item ✅ successfully on 084a140

Thanks for the hint @drmenzelit at #34450 (comment)


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

@eopws
Copy link
Author

eopws commented Jun 8, 2021

Sorry for missing.

According to this issue #34450 it's okay to add horizontal scroll.

But according to this comment #34145 (comment) it's better to break line.

So which is better?

@drmenzelit
Copy link
Contributor

Through few hours of searchin the only one solution that I found is to reduce count of columns, but I'm not sure if it is a good architecture solution (change layout of the whole template in order to make one page look better?)...

I would love to see a comment from a more experienced developer than me...

img

This is the better solution

@eopws
Copy link
Author

eopws commented Jun 9, 2021

Please test new patch.

This is how it looks on my PC
img

@ghost
Copy link

ghost commented Jun 10, 2021

This is how it looks like using latest prebuilt package (custom update server) but i don't know sure if this is now an successfully test:

1500 width:

image

iPad landscape:

image

@angieradtke
Copy link
Contributor

grid2

I think the problem is more complex than the fix .
You changed:

` @include media-breakpoint-up(md) {

grid-template-columns: repeat(4, 1fr);
grid-gap: 1rem 2rem;

}
'

from 4 to 2 columns. But what is width the larger one. This view is broken as well

@include media-breakpoint-up(lg) {
    --span-5: span 5;

    grid-template-columns: repeat(6, 1fr);
  }

The leading-Class column is problem because it goes over two grid column.
@ciar4n , can you take a look please.

@Quy Quy added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Jul 14, 2021
@RickR2H
Copy link
Member

RickR2H commented Oct 14, 2021

I added a PR which I think addresses the same issue as this PR. Please toke a look at my PR an see it id addresses the issue and if it is fixed? #35823. If so, then we can close this PR.


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

@eopws eopws closed this Oct 19, 2021
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 Updates Requested Indicates that this pull request needs an update from the author and should not be tested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants