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 bug with changing focus and apply changes from #157 to SplitButton page, Closes #200 #203

Merged
merged 3 commits into from
Sep 18, 2019

Conversation

marcelwgn
Copy link
Collaborator

Fix a bug where hitting control+b and similar commands will result in navigating the app with focus becoming nearly impossible

Description

Changed the event that was used since the previous event for text colorization would create an endless loop.

I also added the code that was introduced to the RichEditBox in #157 so that the same behavior does not occur in the SplitButton sample.

Motivation and Context

Closes #200

How Has This Been Tested?

Tested manually by starting application, visiting the RichEditBox on the SplitButton page and hitting control+b. After that I was able to successfully navigate to other commands using tab navigation.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@marcelwgn marcelwgn changed the title Fix bug with changing focus and apply changes from #157, Closes #200 Fix bug with changing focus and apply changes from #157 to SplitButton page, Closes #200 Sep 11, 2019
@stmoy stmoy merged commit db96b33 into microsoft:master Sep 18, 2019
@stmoy
Copy link
Contributor

stmoy commented Sep 18, 2019

Philosophical question for you both (@YuliKl @chingucoding) - this change introduces a bunch of codebehind to the sample, but this code isn't included in the sample code shown on the page. Should it be? Or is this just a bunch of "glue" to help make the sample work as expected?

@YuliKl
Copy link

YuliKl commented Sep 18, 2019

Philosophical question for you both (@YuliKl @chingucoding) - this change introduces a bunch of codebehind to the sample, but this code isn't included in the sample code shown on the page. Should it be? Or is this just a bunch of "glue" to help make the sample work as expected?

I definitely think of the code as "glue". It has nothing to do with SplitButton per se, just makes the coloring scenario work better.

@marcelwgn
Copy link
Collaborator Author

I agree with @YuliKl, most of the code is just "glue" to deal with the text changing its color.

One thing we might do is create an empty event listener function. In the sample, the SplitButton "uses" the myColorButton_Click function. Maybe we could add that function with a comment inside saying something like "Handling button click".
However I don't really think that it would be that helpful to just have a C# code block with an empty function. What do you think about that @stmoy and @YuliKl ?

@stmoy
Copy link
Contributor

stmoy commented Sep 18, 2019

However I don't really think that it would be that helpful to just have a C# code block with an empty function. What do you think about that @stmoy and @YuliKl ?

Agreed, I don't think it's super useful. I also agree with the overall direction as Yulia described. Net result: no changes needed.

@marcelwgn marcelwgn deleted the splitbutton-focus-bug branch September 18, 2019 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants