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

Rename extensions with the Extensions suffix (deprecated old, added new) #1765

Merged
merged 13 commits into from
Feb 13, 2018

Conversation

nmetulev
Copy link
Contributor

Issue: #1561

PR Type

What kind of change does this PR introduce?

[x] Code style update (formatting)
[x] Refactoring 

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Docs have been added/updated which fit documentation template. (for bug fixes / features)
  • Sample in sample app has been added / updated (for bug fixes / features)

What is the new behavior?

The following extensions have been renamed

  • ListViewBase -> ListViewExtensions (in addition, the class has been marked static)
  • Hyperlink -> HyperlinkExtensions
  • ApplicationView -> ApplicationViewExtensions
  • StatusBar -> StatusBarExtensions
  • TitleBar -> TitleBarExtensions
  • WebView -> WebViewExtensions
  • VisualEx -> VisualExtensions

Does this PR introduce a breaking change?

[ ] Yes
[x] No - yet (it will in 3.0 when old classes are removed)

@michael-hawker
Copy link
Member

Looks like some files were removed and added vs. moved with git mv? Or is it just GitHub not showing it?

Also, it could be useful to preserve the history for the new copies, but that involves more work (from Robert's answer here):

You can force Git to detect the history of the copied file:
- Instead of copying, switch to a new branch and move the file to its new location there.
- Switch to the original branch and rename the file.
- Merge the new branch into the original branch, resolving the trivial conflict by keeping both files.
- Restore the original filename in a separate commit.
(Solution taken from https://stackoverflow.com/a/44036771/1389680.)

@skendrot
Copy link
Contributor

Shouldn't this be part of the 3.0 release?

@nmetulev
Copy link
Contributor Author

@michael-hawker, you absolutely correct. There are two files that git didn't pick up as moved (and I didn't use git mv). It's the HyperlinkExtensions and ListViewExtensions docs.

@skendrot, this work is to prepare for removing the deprecated extensions in 3.0. I deprecated the existing extensions (but not removed) and created copies with the new name.

@skendrot
Copy link
Contributor

Well that's what happens when you speak without actually looking at the diff!!

@michael-hawker
Copy link
Member

I assume ImageEx stays the same as it's not an extension of image?

@nmetulev
Copy link
Contributor Author

@michael-hawker, correct, it's a control

Copy link
Contributor

@shenchauhan shenchauhan left a comment

Choose a reason for hiding this comment

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

Just a single question on the unwiring of an event. Otherwise it looks good.


if (e.NewValue is string normalizedValue)
{
element.SizeChanged -= KeepCenteredElementSizeChanged;
Copy link
Contributor

Choose a reason for hiding this comment

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

you have already done this on line 606. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed

@azchohfi azchohfi merged commit a3aa6d9 into master Feb 13, 2018
@windowstoolkitbot
Copy link

This PR is linked to unclosed issues. Please check if one of these issues should be closed: #1561

@nmetulev nmetulev deleted the nmetulev/extensions branch February 13, 2018 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants