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

Update "Size and anchors" to reflect Godot 4.3 terminology #10216

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

mikael-ros
Copy link
Contributor

It included terms like "margin" which seem to have been replaced by "Anchor Offsets".

I also replaced the relevant screenshots, but chose not to delete the old ones as I didn't want to accidentally break another article.

This is my second ever contribution to any open source project, so please do have patience with me if I got something wrong. I'll try my best to fix it.

@mikael-ros mikael-ros changed the title Update "Size and anchors" to reflext Godot 4.3 terminology Update "Size and anchors" to reflect Godot 4.3 terminology Nov 5, 2024
Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contributing by updating this page! There are some changes that need to be made, though:

  • You'll need to update the images to preserve their original meaning, and change the format to WebP.
  • There are also a few style changes to make.

The content looks fine, as it's mostly unchanged other than the 3.x->4.x terminology changes.

(Reviewed for style guidelines, but did not deeply review for correctness in 4.x. I believe that anchor offsets still function like margins did, but this will need a review from someone more familiar with UI as well.)

tutorials/ui/img/offset.png Outdated Show resolved Hide resolved
tutorials/ui/img/offsetaround.png Outdated Show resolved Hide resolved
tutorials/ui/img/offsetend.png Outdated Show resolved Hide resolved
tutorials/ui/size_and_anchors.rst Outdated Show resolved Hide resolved
tutorials/ui/size_and_anchors.rst Outdated Show resolved Hide resolved
tutorials/ui/size_and_anchors.rst Outdated Show resolved Hide resolved
tutorials/ui/size_and_anchors.rst Outdated Show resolved Hide resolved
tutorials/ui/size_and_anchors.rst Outdated Show resolved Hide resolved
tutorials/ui/size_and_anchors.rst Outdated Show resolved Hide resolved
tutorials/ui/size_and_anchors.rst Outdated Show resolved Hide resolved
@tetrapod00
Copy link
Contributor

I believe this might close #9950.

@mikael-ros
Copy link
Contributor Author

Thank you for the thorough and respectful comments and suggestions. I will go through them tomorrow when I have time.

I don't know why I didn't realize that the images were baked in collages whilst editing, a bit of a oversight. I'll take and edit new screenshots tomorrow as well :)

@tetrapod00
Copy link
Contributor

I'll take and edit new screenshots tomorrow as well

If you are also taking new screenshots of the control examples, please use fffb44 yellow for the text and lines instead of red, since it's better for colorblindness. See here.

@mikael-ros
Copy link
Contributor Author

Alright, will do. Thank you for the tip! :)

@tetrapod00
Copy link
Contributor

tetrapod00 commented Nov 7, 2024

When I reviewed, I didn't notice this is a duplicate of #10005 and #8855 (all of which try to address #5121).

This PR makes the most minimal changes, only swapping terminiology and updating the screenshots.
#10005 is a larger rewrite of the page. It is not yet reviewed at all.
#8855 is also a larger rewrite, which builds off another PR #7106. It has one review, but not from a maintainer or frequent contributor.

I would tentatively suggest bringing this PR to a mergable state, which would make this page at least not obviously incorrect. Then we can consider salvaging some of the other two PRs to rewrite the page more completely.

@mikael-ros
Copy link
Contributor Author

Should hopefully be up to spec now. Let me know if there's anything else I need to do.

@mikael-ros
Copy link
Contributor Author

I also removed the old images now. I did a search with Visual Studio Code, and it scored no hits on any of the files removed.

@mikael-ros
Copy link
Contributor Author

Hopefully addressed the wordy paragraph I introduced in the original pull request now, too.

@mikael-ros
Copy link
Contributor Author

mikael-ros commented Nov 8, 2024

I have now replaced the transparent images with ones including a backdrop of 4d4d4d, as suggested by @tetrapod00.

Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for making all the changes! This will be reviewed by at least one other reviewer, then it should be good to merge.

Note that at some point you should squash all the commits, as described here. We prefer one commit per PR (though it's less strict in the docs repo). You can either do so now, or after all reviews are done.

For other reviewers: as noted in #10216 (comment) (and the older PRs linked), this page still needs a larger rewrite at some point. This PR makes the page factually correct, but the page is still not really the right tutorial for using anchors and offsets in Godot 4. At some point I'll try to salvage the other two older PRs into a full rewrite.

@mikael-ros
Copy link
Contributor Author

Thank you :) The larger rewrite does sound like a good idea.

I'll look into squashing in a bit. I usually don't squash since I prefer the granularity, but I definetely understand why you'd want a cleaner commit history in such a large project.

@mikael-ros
Copy link
Contributor Author

mikael-ros commented Nov 8, 2024

I'm not sure if this ended up being correct...

It says "x authored and Mikael committed", is that the intended outcome? - it doesn't feel right.

I put fixup on every line I wanted to squash, as the docs recommended. It seems that the page is correct by the latest commit. My main hurdle was that I had merged godotengine:master into my own several times during this several day process, since we've been taught we should merge master into our branch (in this case a fork) when we can - this seems to have muddied it up quite a lot.

Sorry for my naivety. I greatly appreciate your patience with me. Whilst I am studying computer science, they haven't been very rigorous with git past the very basics and cherrypicked examples - so I am very much out of my depth here. I have a backup of my repo in case I need to redo this.

My process went like this:

  1. Synced my master with godotengine:master through the GitHub web interface.
  2. Attempted several squashes locally, ended up removing my local folder and cloning it anew since I messed up (again, locally)
  3. Ran the following commands in my terminal, in the specified order:
git rebase -i eee29a2 # The commit before my first one

This opened Neovim (my assigned editor for git), whereby I edited every commit line that was mine and not the initial commit to be fixup and not pick. All other commits were left on pick. Saved and quit with :wq. Got the response Successfully rebased and updated refs/heads/master..
Then I executed:

git push --force

Yes, I will be the junior dev who invetably ends up nuking the company repo on my first day 😅

@tetrapod00

This comment was marked as resolved.

Changed relevant occurences of margin with "anchor offset" or "offset".

Also reworded paragraph about aspect ratios, as it included information that may not necessarily be correct.

Also replaced images with newer versions.
@mikael-ros
Copy link
Contributor Author

mikael-ros commented Nov 8, 2024

I think it might be correct now. It turned out that my upstream wasn't properly set earlier. Now it was vastly simpler 😅

I had to execute

git remote add upstream [email protected]:godotengine/godot-docs.git
git pull --rebase upstream master # Probably not necessary
git rebase -i upstream/master # As per the guide

At which point I was only presented with my own commits instead of everything else, like intended.

@Calinou Calinou added bug area:manual Issues and PRs related to the Manual/Tutorials section of the documentation topic:gui cherrypick:4.3 labels Nov 8, 2024
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Congratulations for your first merged pull request 🎉

@Calinou Calinou merged commit a362a0a into godotengine:master Nov 8, 2024
1 check passed
@mikael-ros
Copy link
Contributor Author

Thank you :)

tetrapod00 pushed a commit to tetrapod00/godot-docs that referenced this pull request Nov 10, 2024
…ne#10216)

Changed relevant occurences of margin with "anchor offset" or "offset".

Also reworded paragraph about aspect ratios, as it included information that may not necessarily be correct.

Also replaced images with newer versions.
mhilbrunner pushed a commit that referenced this pull request Nov 30, 2024
Changed relevant occurences of margin with "anchor offset" or "offset".

Also reworded paragraph about aspect ratios, as it included information that may not necessarily be correct.

Also replaced images with newer versions.
@mhilbrunner
Copy link
Member

Cherrypicked to 4.3 in #10346.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation bug topic:gui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants