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

Fix changes to widget.Form.Items not being reflected on refresh #5308

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Fix changes to widget.Form.Items not being reflected on refresh #5308

wants to merge 4 commits into from

Conversation

coder-in-go
Copy link

@coder-in-go coder-in-go commented Dec 11, 2024 •
edited by Jacalz
Loading

Description:

This pull request fixes the FormItem replacement issue in the Form Widget

Fixes #1966, #1967

Checklist:

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

@Jacalz Jacalz mentioned this pull request Dec 11, 2024
Closed
Jacalz
Jacalz requested changes Dec 13, 2024
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.

Thanks for the PR. I left two quick comments inline. However, I do also see that this change makes the form tests fail so I think there must be something wrong with what the algorithm is trying to do. Also, it would be nice to add a test for this to verify that it does fix the issues you mention it fixing.

widget/form.go Show resolved Hide resolved
widget/form.go Outdated Show resolved Hide resolved
geodimm
geodimm reviewed Dec 15, 2024
View reviewed changes
widget/form.go Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Dec 16, 2024 •
edited
Loading

Coverage Status

coverage: 59.962% (-0.002%) from 59.964%
when pulling 77640b0 on coder-in-go:bug_fixes/Bug-1966
into 7fea2eb on fyne-io:develop.

@coder-in-go coder-in-go requested a review from Jacalz December 16, 2024 10:01
@coder-in-go coder-in-go reopened this Dec 17, 2024
@Jacalz Jacalz changed the title Fix for issue: 1966: Cannot replace FormItem Widget Fix changes to widget.Form.Items not being reflected on refresh Dec 17, 2024
andydotxyz
andydotxyz reviewed Dec 24, 2024
View reviewed changes
Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Please only use comments in code when
a) the code is not clear without it
b) there is no way to improve the code so that it would be clear.

We expect code to be self-documenting.

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

@geodimm geodimm geodimm left review comments

@andydotxyz andydotxyz andydotxyz left review comments

@Jacalz Jacalz Awaiting requested review from Jacalz

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.
5 participants
@coder-in-go @coveralls @andydotxyz @geodimm @Jacalz