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

Fix: Fixed XAML data binding failure #11574

Merged
merged 5 commits into from Mar 7, 2023
Merged

Fix: Fixed XAML data binding failure #11574

merged 5 commits into from Mar 7, 2023

Conversation

Poker-sang
Copy link
Contributor

@Poker-sang Poker-sang commented Mar 6, 2023

Resolved / Related Issues
Items resolved / related issues by this PR.

  • Closes

Details
When opening a file list, CheckBox.Width's data binding failed for there's no GridLengthToDouble converter

Validation
How did you test these changes?

  • Built and ran the app
  • Tested the changes for accessibility

@Poker-sang Poker-sang changed the title Fix: XAML data binding failed Fix: XAML data binding failure Mar 6, 2023
@yaira2
Copy link
Member

yaira2 commented Mar 6, 2023

How can I reproduce this issue?

@yaira2 yaira2 changed the title Fix: XAML data binding failure Fix: Fixed XAML data binding failure Mar 6, 2023
@Poker-sang
Copy link
Contributor Author

How can I reproduce this issue?

I just build and run the app, and the issue comes up. I guess it can be reproduced when using details view and enabling checkboxes?

Because GridLength cannot convert to double implicitly, I thought this should be fixed sooner or later. 🤔

@yaira2
Copy link
Member

yaira2 commented Mar 6, 2023

I think there is an existing converter that we can use.

marcelwgn
marcelwgn previously approved these changes Mar 6, 2023
Copy link
Contributor

@marcelwgn marcelwgn left a comment

Choose a reason for hiding this comment

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

lgtm

@Poker-sang
Copy link
Contributor Author

I removed the binding, and merged Files.App.Converters and Files.App.ValueConverters namespace. I thought they were duplicated.

@hecksmosis
Copy link
Contributor

I removed the binding, and merged Files.App.Converters and Files.App.ValueConverters namespace. I thought they were duplicated.

This is already being done in #11552

@Poker-sang
Copy link
Contributor Author

This is already being done in #11552

I also simplify some code in the converter classes. But I can restore it if necessary.

@yaira2
Copy link
Member

yaira2 commented Mar 7, 2023

@Poker-sang thank you for your contribution, in general we prefer to limit the scope of PRs to the related issue/feature. As per the contribution guidelines, it's strongly recommended to open an issue and wait for an org member to approve it before opening the PR. This is more for the contributors benefit than ours because it helps everyone be on the same page and avoids duplicate work.
Files is a very active community project, so sticking to these guidelines helps keep track of which work is already in progress and which stuff still need to be picked up 🙂

@Poker-sang
Copy link
Contributor Author

I am sorry. I just encountered a XAML failure and tried to fix it. If this PR was duplicated, please close it. I will pay more attention to this from now on.

@yaira2
Copy link
Member

yaira2 commented Mar 7, 2023

The data binding fix is good but the name spaces changes are being done in another PR.

@Poker-sang
Copy link
Contributor Author

Poker-sang commented Mar 7, 2023

The data binding fix is good but the name spaces changes are being done in another PR.

I will revert it. Thank you.

QuaintMako
QuaintMako previously approved these changes Mar 7, 2023
Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 merged commit 138c147 into files-community:main Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants