Query Loop Block: make elements wrap and use 'gap' instead of 'margin-right' for 'post-template' #36832

shadigaafar · 2021-11-24T17:50:29Z

Description

make elements wrap
use css gap instead of margin-right between elements

How has this been tested?

Screenshots

post-tempate.mp4

Types of changes

#Bug fix (non-breaking change which fixes an issue)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

github-actions · 2021-11-24T17:50:51Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @shadigaafar! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

packages/block-library/src/post-template/editor.scss

remove extra newline Co-authored-by: Zebulan Stanphill <[email protected]>

paaljoachim · 2021-11-24T22:16:56Z

I am thinking it might be helpful if @ntsekouras and @aaronrobertshaw are able to take a closer look at this PR.

aaronrobertshaw · 2021-11-25T00:18:14Z

There are some ongoing efforts aimed at improving the Query Loop Block being tracked in #30910.

I think control over the spacing for the Query Loop Block might be a good candidate for the new block gap support. Perhaps @andrewserong has some thoughts on how easily that might be adopted here?

andrewserong

Thanks for submitting the PR @shadigaafar!

I gave this a quick test, but I wasn't quite sure what the existing issue is that we're trying to resolve here (unfortunately the MP4 file didn't play for me). It looks like this changeset updates the post template so that in all viewports less than 1080px the column count is dynamic (that is, relying on flex to manage how many columns appear). Do you mind updating the PR description to include an example of the problem you're trying to solve?

Unfortunately, the change in this PR seems to override a user setting for column count when folks want to set a lower number of columns (e.g. 2 or 3), and so could affect layouts that are already out in the wild.

I was wondering if this wrap behaviour might be better suited to a block style? I'll just add the Needs design feedback label to this PR, as some designers might have a better idea of the expected behaviour for this block.

Thanks again for submitting the change, and congrats on opening your first Gutenberg PR!

andrewserong · 2021-11-25T01:28:12Z

packages/block-library/src/post-template/style.scss

-				margin-right: 1.25em;
-			}
-
+		@include break-xlarge {


This change effectively switches off the column count for all viewport widths less than 1080px, so if someone has set their Query Loop block to a column count of 2 for example, then in narrower viewports, 3 or 4 columns will be displayed instead.

you are right here, i haven't notice this. this can be fixed. thanks.

andrewserong · 2021-11-25T01:30:40Z

packages/block-library/src/post-template/editor.scss

@@ -4,4 +4,11 @@
 		margin-left: 0;
 		list-style: none;
 	}
+
+	.wp-block-post-template {
+		.is-root-container {


Is this change needed? padding-left and padding-right are already set in the rule above.

yes, there is padding coming from somewhere, this is for WYSIWYG experience.

andrewserong · 2021-11-25T01:33:58Z

packages/block-library/src/post-template/style.scss

@@ -19,24 +19,18 @@
 		flex-direction: row;
 		display: flex;
 		flex-wrap: wrap;
-
+		gap: 1.25em;


If we're going to switch to using gap, like @aaronrobertshaw mentions it might be worth allowing usage of the block gap CSS variable so that we can give users control over the gap between elements. Here's an example of a CSS rule we can use from the Navigation block.

Yes, of course, this what I thought about too, but I thought I could do it in another PR.

ntsekouras · 2021-11-25T08:45:57Z

👋 thanks for the PR @shadigaafar!

but I wasn't quite sure what the existing issue is that we're trying to resolve here (unfortunately the MP4 file didn't play for me)

That goes for me as well.

Unfortunately, the change in this PR seems to override a user setting for column count when folks want to set a lower number of columns (e.g. 2 or 3), and so could affect layouts that are already out in the wild.

Since we have settings for the columns, we can't do this as it will indeed be a breaking change and we will loose the ability to control this.

In my mind for the recent future, Query Loop will be handled with the new Grid layout, which also needs to be implemented.

shadigaafar · 2021-11-25T09:59:09Z

@andrewserong @ntsekouras
The problem is obvious, if you think this view is good and pleasant, then there is no problem here.

above this, currently, this does not quite work in RTL, the margin-right is not changing to margin-left when RTL, so i thought instead of flipping to margin-left for RTL, using gap for both is good, and could allow for future control for the gap, and it will solve the problem of wraping , cos you can't wrap with margins. not a good idea.

shadigaafar · 2021-11-25T10:28:06Z

In my mind for the recent future, Query Loop will be handled with the new Grid layout, which also needs to be implemented

And please, don't talk about future too much, like we say in my homeland: "when it will born, we will name it". something is not here yet; we don't talk about it. we can't just keep things broken, hoping that the future will fix this. there should at least a temporary fix.

as far as I have noticed, that the future will not fix anything, cos no one thinks there is a problem to be fix. if you can't see the problem, how you are going to fix it in the future.

I'm very disappointed not because there is some issue with my PR, but that no one thought that this PR, adds any important value.

ntsekouras · 2021-11-25T11:14:26Z

And please, don't talk about future too much, like we say in my homeland: "when it will born, we will name it". something is not here yet; we don't talk about it. we can't just keep things broken, hoping that the future will fix this. there should at least a temporary fix.

There always should be a balance when we introduce a temporary fix, as this just increases complexity for the project and technical debt. First we should explore the best possible solution and also a temporary fix cannot introduce breaking changes.

as far as I have noticed, that the future will not fix anything, cos no one thinks there is a problem to be fix. if you can't see the problem, how you are going to fix it in the future.

I don't agree with this to be honest. It's not possible for everyone to see every possible problem, but when one sees a problem should create an issue for it. That's the way for others to become aware of this problem too and work towards its resolution.

I'm very disappointed not because there is some issue with my PR, but that no one thought that this PR, adds any important value.

Every contribution is valued and appreciated, including this one! This doesn't mean that other contributors can't express their thoughts, questions and concerns, no matter with the final outcome of the discussion.

paaljoachim · 2021-11-25T11:21:48Z

I just want to say that sometimes issues or PR's because of various reasons might take some time to get movement or agreed upon. It just happens. Sometimes we just need to pause a solution. I would then need to move on to other things and circle back at another time. Please do not let it stop you from contributing.

shadigaafar · 2021-11-25T11:37:31Z

Yet, you did not give an answer, What now? should I fix the problem with this PR? or should i just forget about it? What about the RTL related issue? should we leave it for now?

priethor · 2021-11-25T12:35:39Z

Hi @shadigaafar , thanks for raising this issue and opening your first PR! 🎉

As other contributors have already mentioned, this PR introduces conflicts in other use cases and raises the overall complexity. This doesn't mean there is no issue to solve and the PR should not move forward, but that it will need further iterations and reviews before landing so that we can ensure we fix the issue while not introducing new ones.

You are more than welcome to continue working on this PR. We will try to provide support and guidance as best as we can, although it's worth noting we are right in the middle of a release crunch so contributors' time is a bit scarce, as the current focus is addressing the WordPress 5.9 pending issues listed in #36556.

In order for other folks to help understand the issue at hand, would you mind re-uploading a working version of the video in the opening message of this PR? This will greatly help get a better understanding for other people, easing the process to help with this PR.

t-hamano · 2023-12-18T03:34:18Z

Hi everyone. I came across this PR while checking for PRs whose status hasn't been updated in a long time.

I found that #37711, which applies a grid in gridview, has already been merged. Does #37711 cover what this PR is doing?

carolinan · 2024-12-06T06:41:50Z

It as been close to a year with no response and no updates so I am closing this pull request.

make elements wrap and use 'gap' instead of margin for 'post-template'

524221d

shadigaafar requested a review from ajitbohra as a code owner November 24, 2021 17:50

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Nov 24, 2021

ZebulanStanphill added [Block] Post Template Affects the Post Template Block CSS Styling Related to editor and front end styles, CSS-specific issues. labels Nov 24, 2021

ZebulanStanphill requested a review from kjellr November 24, 2021 18:57

ZebulanStanphill reviewed Nov 24, 2021

View reviewed changes

packages/block-library/src/post-template/editor.scss Outdated Show resolved Hide resolved

ZebulanStanphill added the [Type] Enhancement A suggestion for improvement. label Nov 24, 2021

Update packages/block-library/src/post-template/editor.scss

a910c6c

remove extra newline Co-authored-by: Zebulan Stanphill <[email protected]>

andrewserong reviewed Nov 25, 2021

View reviewed changes

andrewserong added the Needs Design Feedback Needs general design feedback. label Nov 25, 2021

carolinan closed this Dec 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query Loop Block: make elements wrap and use 'gap' instead of 'margin-right' for 'post-template' #36832

Query Loop Block: make elements wrap and use 'gap' instead of 'margin-right' for 'post-template' #36832

shadigaafar commented Nov 24, 2021 •

edited

Loading

github-actions bot commented Nov 24, 2021

paaljoachim commented Nov 24, 2021

aaronrobertshaw commented Nov 25, 2021

andrewserong left a comment

andrewserong Nov 25, 2021

shadigaafar Nov 25, 2021

andrewserong Nov 25, 2021

shadigaafar Nov 25, 2021

andrewserong Nov 25, 2021

shadigaafar Nov 25, 2021 •

edited

Loading

ntsekouras commented Nov 25, 2021

shadigaafar commented Nov 25, 2021 •

edited

Loading

shadigaafar commented Nov 25, 2021 •

edited

Loading

ntsekouras commented Nov 25, 2021

paaljoachim commented Nov 25, 2021 •

edited

Loading

shadigaafar commented Nov 25, 2021

priethor commented Nov 25, 2021

t-hamano commented Dec 18, 2023

carolinan commented Dec 6, 2024

Query Loop Block: make elements wrap and use 'gap' instead of 'margin-right' for 'post-template' #36832

Query Loop Block: make elements wrap and use 'gap' instead of 'margin-right' for 'post-template' #36832

Conversation

shadigaafar commented Nov 24, 2021 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

github-actions bot commented Nov 24, 2021

paaljoachim commented Nov 24, 2021

aaronrobertshaw commented Nov 25, 2021

andrewserong left a comment

Choose a reason for hiding this comment

andrewserong Nov 25, 2021

Choose a reason for hiding this comment

shadigaafar Nov 25, 2021

Choose a reason for hiding this comment

andrewserong Nov 25, 2021

Choose a reason for hiding this comment

shadigaafar Nov 25, 2021

Choose a reason for hiding this comment

andrewserong Nov 25, 2021

Choose a reason for hiding this comment

shadigaafar Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

ntsekouras commented Nov 25, 2021

shadigaafar commented Nov 25, 2021 • edited Loading

shadigaafar commented Nov 25, 2021 • edited Loading

ntsekouras commented Nov 25, 2021

paaljoachim commented Nov 25, 2021 • edited Loading

shadigaafar commented Nov 25, 2021

priethor commented Nov 25, 2021

t-hamano commented Dec 18, 2023

carolinan commented Dec 6, 2024

shadigaafar commented Nov 24, 2021 •

edited

Loading

shadigaafar Nov 25, 2021 •

edited

Loading

shadigaafar commented Nov 25, 2021 •

edited

Loading

shadigaafar commented Nov 25, 2021 •

edited

Loading

paaljoachim commented Nov 25, 2021 •

edited

Loading