Skip to content

Fix "Italic" checkstate bug + fix "italic" typo in InfiniteCanvas control #3117

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
Merged
merged 4 commits into from
Mar 24, 2020
Merged

Fix "Italic" checkstate bug + fix "italic" typo in InfiniteCanvas control #3117

merged 4 commits into from
Mar 24, 2020

Conversation

BurkusCat
Copy link
Contributor

@BurkusCat BurkusCat commented Jan 31, 2020 •
edited
Loading

  1. Fix Re-selecting an InfiniteCanvas text node can show incorrect Italics state in certain situations #3118 - Re-selecting an InfiniteCanvas text node can show incorrect Italics state in certain situations
  2. Fix a small #misspelling in the InfiniteCanvas control

PR Type

What kind of change does this PR introduce?

Bugfix

Code style update (formatting)

What is the current behavior?

  1. Italic checkstate can get out of sync when re-selecting a previous text node
  2. Variable names mispelled

What is the new behavior?

  1. Italic check state stays in sync when re-selecting previous text nodes
  2. Variable names correctly spelled

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • 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

@BurkusCat
Fix "italic" typo
693353f 
itlaic -> italic
@ghost
Copy link

ghost commented Jan 31, 2020

Thanks BurkusCat 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 assigned azchohfi Jan 31, 2020
@dnfclas
Copy link

dnfclas commented Jan 31, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@BurkusCat BurkusCat force-pushed the fix-italic-spelling branch from f5370c5 to a4d7309 Compare January 31, 2020 22:52
@BurkusCat BurkusCat changed the title Fix "italic" typo in InfiniteCanvas control Fix "Italic" checkstate bug + fix "italic" typo in InfiniteCanvas control Jan 31, 2020
Kyaa-dost
Kyaa-dost approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@Kyaa-dost Kyaa-dost 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 fix @BurkusCat. The changes look good!

michael-hawker
michael-hawker approved these changes Feb 9, 2020
View reviewed changes
Microsoft.Toolkit.Uwp.UI.Controls/InfiniteCanvas/InfiniteCanvas.cs
@@ -166,7 +166,7 @@ protected override void OnApplyTemplate()
_canvasTextBoxTools = (StackPanel)GetTemplateChild("CanvasTextBoxTools");
_canvasTextBoxColorPicker = (ColorPicker)GetTemplateChild("CanvasTextBoxColorPicker");
_canvasTextBoxFontSizeTextBox = (TextBox)GetTemplateChild("CanvasTextBoxFontSizeTextBox");
_canvasTextBoxItlaicButton = (ToggleButton)GetTemplateChild("CanvasTextBoxItlaicButton");
_canvasTextBoxItalicButton = (ToggleButton)GetTemplateChild("CanvasTextBoxItalicButton");
Copy link
Member

Choose a reason for hiding this comment

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

I was going to say changing the name in the Style is going to be a breaking change, but we don't actually call them out as TemplateParts like we should... (for instance see TabView).

@BurkusCat would you be up for fixing that too? Let me know if you want to do it as part of this change or another.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michael-hawker I can take a look at that next weekend.

Copy link
Member

Choose a reason for hiding this comment

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

@BurkasCat I'm approving this PR, just let me know, but we can merge this one first, and just open another one for those changes if you get to them. Thanks for the contribution! 🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

@BurkusCat Is there any update on this or still being worked on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kyaa-dost Apologies, I did not get a chance to look at this. I can try take a look at this on this weekend if it suits.

@ghost ghost added the no-recent-activity 📉 Open Issues that require attention label Mar 24, 2020
@ghost
Copy link

ghost commented Mar 24, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 30 days of this comment.

@ghost ghost removed the no-recent-activity 📉 Open Issues that require attention label Mar 24, 2020
azchohfi
azchohfi approved these changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@azchohfi azchohfi left a comment

Choose a reason for hiding this comment

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

We can fix the template part in another PR. Let's just merge this. It looks good.

@BurkusCat
Copy link
Contributor Author

BurkusCat commented Mar 24, 2020 •
edited
Loading

Sorry, hectic times as you can imagine 🙈 Thanks for approving!

@michael-hawker
Copy link
Member

@BurkusCat no worries, that was my intent, we'll get this merged in now, and I'll quickly do the TemplatePart. I'm actually working on this on a live stream at the moment now :)

@michael-hawker michael-hawker merged commit 2dd783d into CommunityToolkit:master Mar 24, 2020
@michael-hawker michael-hawker mentioned this pull request Mar 27, 2020
Closed
36 tasks
@Kyaa-dost Kyaa-dost added bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️ labels Jun 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kyaa-dost Kyaa-dost Kyaa-dost approved these changes

@azchohfi azchohfi azchohfi approved these changes

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

@IbraheemOsama IbraheemOsama Awaiting requested review from IbraheemOsama

Assignees

@azchohfi azchohfi

Labels
bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Re-selecting an InfiniteCanvas text node can show incorrect Italics state in certain situations
5 participants
@BurkusCat @dnfclas @michael-hawker @azchohfi @Kyaa-dost