8000 Fix ColorPickerButton SelectedColor bug by Marv51 · Pull Request #4372 · CommunityToolkit/WindowsCommunityToolkit · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix ColorPickerButton SelectedColor bug #4372

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

Jump to bottom
Merged
merged 3 commits into from
Jan 19, 2022
Merged

Fix ColorPickerButton SelectedColor bug #4372

merged 3 commits into from
Jan 19, 2022

Conversation

Marv51
Copy link
Contributor
@Marv51 Marv51 commented Nov 9, 2021 •
edited
Loading

Fixes #4367

Update internal ColorPicker Color when ColorPickerButton SelectedColor changes

PR Type

What kind of change does this PR introduce?

Bugfix

What is the current behavior?

When ColorPickerButton.SelectedColor Property changes, the changed color is not the one in the colorpicker when the user clicks on the ColorPickerButton

What is the new behavior?

SelectedColor gets propagated to the internal colorpicker to fix this bug.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • New component
    • Pull Request has been submitted to the documentation repository instructions. Link:
    • Added description of major feature to project description for NuGet package (4000 total character limit, so don't push entire description over that)
    • If control, added to Visual Studio Design project
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

I am unfamiliar with all the testing infrastructure you have in place, so I just created little sample app for myself with the following two snippets. Should I add those somewhere?

       <StackPanel>
           <controls:ColorPickerButton SelectedColor="{x:Bind TheColor, Mode=TwoWay}" />
           <Button Click="Button_Click">Set color to red</Button>
       </StackPanel>
private Color _theColor = Colors.Green;
public Color TheColor
{
    get => _theColor;
    set
    {
        if (_theColor != value)
        {
            _theColor = value;
            OnPropertyChanged(nameof(TheColor));
        }
    }
}

public MainPage()
{
    DataContext = this;
    this.InitializeComponent();
}

public event PropertyChangedEventHandler PropertyChanged;

private void OnPropertyChanged(string propertyName) => PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));

private void Button_Click(object sender, RoutedEventArgs e)
{
    TheColor = Colors.Red;
}

@Marv51
Fix ColorPickerButton SelectedColor bug (fixes CommunityToolkit#4367)
b4c6fdc 
Update internal ColorPicker Color when ColorPickerButton SelectedColor changes
@ghost
Copy link
ghost commented Nov 9, 2021

Thanks Marv51 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker and azchohfi November 9, 2021 13:57
@ghost ghost added the bug 🐛 An unexpected issue that highlights incorrect behavior label Nov 9, 2021
XAML-Knight
XAML-Knight approved these changes Nov 10, 2021
View reviewed changes
Copy link
Contributor
@XAML-Knight XAML-Knight left a comment

Choose a reason for hiding this comment

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

PR approved.

If you're interested in authoring a unit test, take a look at a similar test (UITests\UITests.Tests.Shared\RichSuggestBoxTest.cs); UI test guidance here.

@Marv51
Copy link
Contributor Author
Marv51 commented Nov 11, 2021

Hi @XAML-Knight, thanks for pointing me in the right direction to get started with the UITest. I added a test to automate the manual test I described in the PR description.

I found the "Accessibility Insights for Windows" tool very useful to inspect the UI, it might be worth mentioning on that wiki page.

@Marv51 @XAML-Knight
Update UITests/UITests.Tests.Shared/Controls/ColorPickerButtonTest.cs
8fcb64a 
Co-authored-by: XAML-Knight <86266896+XAML-Knight@users.noreply.github.com>
@ghost
Copy link
ghost commented Nov 12, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@Marv51
Copy link
Contributor Author
Marv51 commented Dec 7, 2021

Any update on this? Is there anything else I need to do?

@michael-hawker michael-hawker added this to the 7.2/8.0? milestone Dec 7, 2021
michael-hawker
michael-hawker approved these changes Dec 7, 2021
View reviewed changes
Copy link
Member
@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Think this looks good. @Arlodotexe want to take a quick look to?

@Arlodotexe
Copy link
Member

Looks good to me!

@Marv51
Copy link
Contributor Author
Marv51 commented Dec 13, 2021

Sounds like this was approved by everyone? So could someone with write-access merge this or should we loop in anyone else?

@michael-hawker
Copy link
Member

Sorry @Marv51 this should have gotten merged. Doing it now.

@michael-hawker michael-hawker merged commit 4fd5702 into CommunityToolkit:main Jan 19, 2022
@michael-hawker michael-hawker modified the milestones: 8.0, 7.1.3 Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arlodotexe Arlodotexe Arlodotexe approved these changes

@michael-hawker michael-hawker michael-hawker approved these changes

@XAML-Knight XAML-Knight XAML-Knight approved these changes

@azchohfi azchohfi Awaiting requested review from azchohfi

Assignees
No one assigned
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️
Projects
None yet
Milestone
7.1.3
Development

Successfully merging this pull request may close these issues.

ColorPickerButton SelectedColor binding not updating correctly
4 participants
@Marv51 @Arlodotexe @michael-hawker @XAML-Knight
0