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

Update the padding of text under a form item #4149

Merged
merged 4 commits into from
Aug 12, 2023
Merged

Update the padding of text under a form item #4149

merged 4 commits into from
Aug 12, 2023

Conversation

andydotxyz
Copy link
Member

Fixes #4137

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Jacalz
Jacalz requested changes Aug 11, 2023
View reviewed changes
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

I pushed a commit to fix the tests. Just one inline comment about a variable name and a minor cleanup while you're at it.

widget/form.go
min0 := objs[0].MinSize()
min1 := objs[1].MinSize()

minWidth := fyne.Max(min0.Width, min1.Width)
Copy link
Member

Choose a reason for hiding this comment

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

Should this not be called "maxWidth"?

Copy link
Member Author

@andydotxyz andydotxyz Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No, the max of to minimums is still a minimum - computing the max width inside the MinSize method would be weird ;).

widget/form.go Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Aug 11, 2023 •
edited
Loading

Coverage Status

coverage: 65.196% (-0.009%) from 65.205% when pulling 78d8d09 on andydotxyz:fix/4137 into 90192cc on fyne-io:develop.

@andydotxyz @Jacalz
Update widget/form.go
78d8d09 
Co-authored-by: Jacob Alzén <[email protected]>
@andydotxyz
Copy link
Member Author

I pushed a commit to fix the tests. Just one inline comment about a variable name and a minor cleanup while you're at it.

Thanks - between replying and accepting the simplification I think we're good?

@andydotxyz andydotxyz requested a review from Jacalz August 12, 2023 04:37
Jacalz
Jacalz approved these changes Aug 12, 2023
View reviewed changes
Copy link
Member

@Jacalz Jacalz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I thought I had approved

@andydotxyz andydotxyz merged commit 68f40a0 into fyne-io:develop Aug 12, 2023
11 checks passed
@andydotxyz andydotxyz deleted the fix/4137 branch August 12, 2023 07:27
@pseidemann
Copy link

thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jacalz Jacalz Jacalz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andydotxyz @coveralls @pseidemann @Jacalz