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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Document Size and UiRect #5381

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Document Size and UiRect #5381

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 19, 2022

Objective

Solution

  • Document Size and UiRect.
  • I also removed the type alias from the size_ops test since it's unnecessary.

Follow Up

After this change is merged I'd follow up with removing the generics from Size and UiRect since Val should be extensible enough. This was also discussed and decided on in #3503. let me know if this is not needed or wanted anymore!

@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Docs An addition or correction to our documentation labels Jul 19, 2022
alice-i-cecile
alice-i-cecile approved these changes Jul 19, 2022
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

These are excellent. Ping @Weibye @TimJentzsch for a review or two :)

TimJentzsch
TimJentzsch reviewed Jul 20, 2022
View reviewed changes
Copy link
Contributor

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

In general I really like the docs!

Some things I'd like to discuss:

  • For some examples, all the values for left, right, top and bottom are mentioned again at the text, even though they are already specified clearly in the code. I'm wondering if that is necessary, i.e. if it adds additional value. If not I'd prefer to leave it out in favor of shorter docs. I'd ve curious to hear @alice-i-cecile's and @Weibye's opinion on this.
  • All of the examples are with pixel units, it might make sense to also include some with percentages (or other units if we have more), to demonstrate that this is possible.

crates/bevy_ui/src/geometry.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/geometry.rs Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member

For some examples, all the values for left, right, top and bottom are mentioned again at the text, even though they are already specified clearly in the code. I'm wondering if that is necessary, i.e. if it adds additional value. If not I'd prefer to leave it out in favor of shorter docs. I'd ve curious to hear @alice-i-cecile's and @Weibye's opinion on this.

I think we should cut this. It's a bit more verbose, but the critical thing is that this is duplication that isn't automatically checked for us by the compiler, and risks getting desynced.

All of the examples are with pixel units, it might make sense to also include some with percentages (or other units if we have more), to demonstrate that this is possible.

Firmly agree.

@ghost
Copy link
Author

ghost commented Jul 20, 2022

For some examples, all the values for left, right, top and bottom are mentioned again at the text, even though they are already specified clearly in the code. I'm wondering if that is necessary, i.e. if it adds additional value. If not I'd prefer to leave it out in favor of shorter docs. I'd ve curious to hear @alice-i-cecile's and @Weibye's opinion on this.

Removed!

All of the examples are with pixel units, it might make sense to also include some with percentages (or other units if we have more), to demonstrate that this is possible.

I changed some of the Val types from Px to Auto or Percent where it made sense. Most of the time Px values are used, but I found some nice examples like UiRect::all(Val::Auto) for margins which centers the UI element. While searching for some example usage of padding in bevy I couldn't find a single example using it so I left it as is for now (seems to be a popular setting lol).

alice-i-cecile
alice-i-cecile approved these changes Jul 20, 2022
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot pushed a commit that referenced this pull request Jul 20, 2022
Document Size and UiRect (#5381)
60c6934 
# Objective

- Migrate changes from #3503.

## Solution

- Document `Size` and `UiRect`.
- I also removed the type alias from the `size_ops` test since it's unnecessary.

## Follow Up

After this change is merged I'd follow up with removing the generics from `Size` and `UiRect` since `Val` should be extensible enough. This was also discussed and decided on in #3503. let me know if this is not needed or wanted anymore!
@bors
Copy link
Contributor

bors bot commented Jul 20, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Document Size and UiRect [Merged by Bors] - Document Size and UiRect Jul 20, 2022
@bors bors bot closed this Jul 20, 2022
@ghost ghost deleted the document_size_and_ui_rect branch August 7, 2022 08:27
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
@inodentry
Document Size and UiRect (bevyengine#5381)
0b293c3 
# Objective

- Migrate changes from bevyengine#3503.

## Solution

- Document `Size` and `UiRect`.
- I also removed the type alias from the `size_ops` test since it's unnecessary.

## Follow Up

After this change is merged I'd follow up with removing the generics from `Size` and `UiRect` since `Val` should be extensible enough. This was also discussed and decided on in bevyengine#3503. let me know if this is not needed or wanted anymore!
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@james7132
Document Size and UiRect (bevyengine#5381)
c9315fb 
# Objective

- Migrate changes from bevyengine#3503.

## Solution

- Document `Size` and `UiRect`.
- I also removed the type alias from the `size_ops` test since it's unnecessary.

## Follow Up

After this change is merged I'd follow up with removing the generics from `Size` and `UiRect` since `Val` should be extensible enough. This was also discussed and decided on in bevyengine#3503. let me know if this is not needed or wanted anymore!
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@ItsDoot
Document Size and UiRect (bevyengine#5381)
fd42022 
# Objective

- Migrate changes from bevyengine#3503.

## Solution

- Document `Size` and `UiRect`.
- I also removed the type alias from the `size_ops` test since it's unnecessary.

## Follow Up

After this change is merged I'd follow up with removing the generics from `Size` and `UiRect` since `Val` should be extensible enough. This was also discussed and decided on in bevyengine#3503. let me know if this is not needed or wanted anymore!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Docs An addition or correction to our documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alice-i-cecile @TimJentzsch