Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Conversation

shana
Copy link
Contributor

@shana shana commented Sep 1, 2015

Fixes #62

This fixes the checkbox styling so it still matches Team Explorer but doesn't break our reactive bindings.

The previous style was using an inner checkbox for the rendering, and when reactiveui binds the view to the viewmodel, it likely looks for changed properties in the RoutedEventArgs Source or possibly the sender object that goes along with the event, and both of these represent the outer checkbox, which is not the originator of the Checked event, so it doesn't actually have the IsChecked property set.

This caused the UI to appear correct (inner checkbox is set), but the binding to not pick up the change (outer checkbox not set).

@shiftkey
Copy link
Member

shiftkey commented Sep 1, 2015

Pulled this down to test, and yes I can see the checkbox state updating the VM property correctly ✨

@shana anything else you'd like us to focus on with the review?

@shana
Copy link
Contributor Author

shana commented Sep 1, 2015

@shiftkey One of the reasons things broke is that I tried to make a style combining the CheckBox and the TextBlock without recreating the checkbox style (because I have no clue what it is). Is there a better way of doing this than what I'm doing now (adding the textblock inside the checkbox directly in the xaml publish control)?

Copy link
Member

Choose a reason for hiding this comment

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

You can strip this back to just the literal Private Repository element - this will get rendered as an inline TextBlock and pick up the styling...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right, I can move the trigger up to the checkbox

/me fixicates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sigh, can't merge the TextBlock. If I do, then the Hover color in the checkbox (turning the square border a different color) also gets applied to the text. Which is something that the TE styles are not doing, it seems.

Copy link
Member

Choose a reason for hiding this comment

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

Uggghh 👍

@shana
Copy link
Contributor Author

shana commented Sep 8, 2015

@haacked 👀 👍 👎?

haacked added a commit that referenced this pull request Sep 8, 2015
@haacked haacked merged commit 0cd7005 into master Sep 8, 2015
@haacked haacked deleted the shana/62-fix-checkbox branch September 8, 2015 21:38
@haacked
Copy link
Contributor

haacked commented Sep 8, 2015

If @shiftkey gives a 👍 then I'm a 👍!

@shana shana modified the milestone: 1.0.15 Sep 16, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create Private Repo creates public repository

3 participants