Skip to content

Add vertical property to GridContainers #98681

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RLCcar
Copy link

@RLCcar RLCcar commented Oct 30, 2024

Grid Containers could only set a fixed number of vertical columns, which restricts its use on shorter, wider screens, compared to other containers with Horizontal/Vertical counterparts.

By adding a vertical property to Grid Containers, the user can choose to fix the number of rows OR columns in the grid.

Bugsquad edit:

@RLCcar RLCcar requested review from a team as code owners October 30, 2024 21:06
@Calinou Calinou added this to the 4.x milestone Oct 31, 2024
@RLCcar RLCcar force-pushed the horizontal-grid-container branch from 63c8652 to 2c675a1 Compare October 31, 2024 06:26
@AThousandShips AThousandShips changed the title Added vertical property to GridContainers Add vertical property to GridContainers Oct 31, 2024
@RLCcar
Copy link
Author

RLCcar commented Nov 1, 2024

I'm trying to fix some formatting issues in the .xml and .svg files to meet the static style checks, but processing the new .svg files gives me this error:

\ No newline at end of file

I used svgcleaner to optimize the files, and I checked the line endings. Is there a style guide or is something missing?

@ydeltastar
Copy link
Contributor

ydeltastar commented Nov 1, 2024

The new line message is just verbose. The output shows a diff with the correct format which moves the parameters in <svg ...>.

-<svg height="16" width="16" xmlns="http://www.w3.org/2000/svg"><path fill="#8eef97" d="m3 1c-1.1045695 0-2 .8954305-2 2v10c0 1.104569.8954305 2 2 2h10c1.104569 0 2-.895431 2-2v-10c0-1.1045695-.895431-2-2-2zm0 2h2v4h-2zm4 0h2v4h-2zm4 0h2v4h-2zm-8 6h2v4h-2zm4 0h2v4h-2zm4 0h2v4h-2z"/></svg>
+<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16"><path fill="#8eef97" d="m3 1c-1.1045695 0-2 .8954305-2 2v10c0 1.104569.8954305 2 2 2h10c1.104569 0 2-.895431 2-2v-10c0-1.1045695-.895431-2-2-2zm0 2h2v4h-2zm4 0h2v4h-2zm4 0h2v4h-2zm-8 6h2v4h-2zm4 0h2v4h-2zm4 0h2v4h-2z"/></svg>

The optimizer should be doing it. Looks like they switched from svgcleaner to svgo but didn't change it in https://docs.godotengine.org/en/latest/contributing/development/editor/creating_icons.html.

@RLCcar RLCcar force-pushed the horizontal-grid-container branch from 2c675a1 to 218ba43 Compare November 3, 2024 21:27
@RLCcar
Copy link
Author

RLCcar commented Nov 7, 2024

It appears Static checks and Linux / Editor w/ Mono have contradicting complaints with the XML documentation.

Static checks required that the HGridContainer and the VGridContainer XML contained definitions for the rows and columns variables, but Linux / Editor w/ Mono demands I remove them.

Is it a deeper issue with how the variables are configured, or an issue with Linux Mono?

@Calinou
Copy link
Member

Calinou commented Nov 7, 2024

The CI log says:

Running --doctool to see if this changes the public API without updating the documentation.
If a diff is shown, it means that your code/doc changes are incomplete and you should update the class reference with --doctool.

Compile a Godot binary locally and run the --doctool command line argument on it to update the class reference XML locally. After doing this, git commit -a --amend your changes.

@RLCcar RLCcar force-pushed the horizontal-grid-container branch from 218ba43 to 28753b2 Compare November 8, 2024 05:12
@RLCcar
Copy link
Author

RLCcar commented Nov 8, 2024

It appears Static checks and Linux / Editor w/ Mono have contradicting complaints with the XML documentation.

Static checks required that the HGridContainer and the VGridContainer XML contained definitions for the rows and columns variables, but Linux / Editor w/ Mono demands I remove them.

I've ran the --doctool command and pushed my new changes, but the original problem still stands: Static Checks and Linux / Editor w/ Mono contradict each other.

I'm trying to figure out why Static Checks demands that HBoxContainer and VGridContainer must include GridContainer properties in the documentation, even though it doesn't for other classes with horizontal/vertical variants. Am I missing something for the new classes?

@adamscott
Copy link
Member

adamscott commented Nov 8, 2024

It appears Static checks and Linux / Editor w/ Mono have contradicting complaints with the XML documentation.
Static checks required that the HGridContainer and the VGridContainer XML contained definitions for the rows and columns variables, but Linux / Editor w/ Mono demands I remove them.

I've ran the --doctool command and pushed my new changes, but the original problem still stands: Static Checks and Linux / Editor w/ Mono contradict each other.

I'm trying to figure out why Static Checks demands that HBoxContainer and VGridContainer must include GridContainer properties in the documentation, even though it doesn't for other classes with horizontal/vertical variants. Am I missing something for the new classes?

I just found out. That's because the checks think that you refer to members {H,V}BoxContainer.{rows,columns}, but they don't exist. You need to specify that the member is from another class. Here's the diff to correct your issues:

diff --git a/doc/classes/HGridContainer.xml b/doc/classes/HGridContainer.xml
index 09f113035f..8a7cf007be 100644
--- a/doc/classes/HGridContainer.xml
+++ b/doc/classes/HGridContainer.xml
@@ -4,7 +4,7 @@
 		A container that arranges its child controls in a grid layout horizontally.
 	</brief_description>
 	<description>
-		A variant of [GridContainer] that can only arrange its child controls horizontally, with a set number of [member rows], and a changing number of [member columns].
+		A variant of [GridContainer] that can only arrange its child controls horizontally, with a set number of [member GridContainer.rows], and a changing number of [member GridContainer.columns].
 		Child controls are rearranged automatically when their minimum size changes.
 	</description>
 	<tutorials>
diff --git a/doc/classes/VGridContainer.xml b/doc/classes/VGridContainer.xml
index 5eb54b4cbf..fb86b3e41a 100644
--- a/doc/classes/VGridContainer.xml
+++ b/doc/classes/VGridContainer.xml
@@ -4,7 +4,7 @@
 		A container that arranges its child controls in a grid layout vertically.
 	</brief_description>
 	<description>
-		A variant of [GridContainer] that can only arrange its child controls vertically, with a set number of [member columns], and a changing number of [member rows].
+		A variant of [GridContainer] that can only arrange its child controls vertically, with a set number of [member GridContainer.columns], and a changing number of [member GridContainer.rows].
 		Child controls are rearranged automatically when their minimum size changes.
 	</description>
 	<tutorials>

@adamscott
Copy link
Member

The diff should be good now. It included too much stuff from when I was experimenting at first. Sorry.

@Mickeon
Copy link
Member

Mickeon commented Nov 8, 2024

I understand it's there for consistency with existing containers, but is there actually a need for H and V versions of GridContainer too?

@RLCcar
Copy link
Author

RLCcar commented Nov 8, 2024

I understand it's there for consistency with existing containers, but is there actually a need for H and V versions of GridContainer too?

It's there mainly for consistency with the other containers. Plus, the H and V versions make it clearer how the GridContainer is being used.

@RLCcar RLCcar force-pushed the horizontal-grid-container branch from 28753b2 to 17c36d0 Compare November 8, 2024 17:08
@KoBeWi

This comment was marked as resolved.

@RLCcar RLCcar force-pushed the horizontal-grid-container branch from 17c36d0 to 66b218a Compare December 17, 2024 17:39
@RLCcar RLCcar force-pushed the horizontal-grid-container branch from 66b218a to 1268b90 Compare January 14, 2025 20:11
@RLCcar RLCcar force-pushed the horizontal-grid-container branch from d6c07a0 to 4dd10b1 Compare August 7, 2025 17:43
@RLCcar RLCcar force-pushed the horizontal-grid-container branch from 4dd10b1 to 7ef9325 Compare August 7, 2025 21:07
@RLCcar
Copy link
Author

RLCcar commented Aug 7, 2025

For some reason, Git is preventing me from changing the line endings of the following SVG files from CRLF to LF:

  • HGridContainer.svg
  • VGridContainer.svg

There is no newline at the end of the file, so it fails the first test. Can I get some help fixing these?

@Calinou
Copy link
Member

Calinou commented Aug 7, 2025

You need to add a LF line ending at the end of each file (currently, there is none). I suggest configuring your editor to always use LF line endings, then modifying the SVGs in question.

Many editors also have a "Ensure final newline on save" setting I suggest enabling too.

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

Successfully merging this pull request may close these issues.

Add a vertical property to GridContainer
7 participants