-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 two crashes from NativeAnimatedExamples #3400
Fix two crashes from NativeAnimatedExamples #3400
Conversation
1) you cannot animated 2 subchannels of the same property with different animations. to fix this we animated yet another property set for translation and scale owned by the props nodes and use one animation to animate all of the subchannels for the uiElement. 2) Reference parameter names which started with a multi digit number are unsupported so i added an n to the start of each name, which was previously just the node's tag.
…g them to double count the start value.
@@ -0,0 +1,34 @@ | |||
#pragma once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this after Copyright #Resolved
#include "pch.h" | ||
|
||
#include <Views/ShadowNodeBase.h> | ||
#include "SliderViewManager.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include <Views/SliderViewManager.h> ? #Pending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not how any of the other view managers include their header
In reply to: 335125441 [](ancestors = 335125441)
Super::createView(); | ||
|
||
auto slider = GetView().as<winrt::Slider>(); | ||
auto wkinstance = GetViewManager()->GetReactInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You create them but you never use them. please just remove them.
I guess you copied it from other controls, please make change to other control too. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
void SliderShadowNode::updateProperties(const folly::dynamic &&props) { | ||
m_updating = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_updating [](start = 2, length = 10)
why do you need m_updating flag? #Pending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the shadow nodes do this, i couldn't tell you why
In reply to: 335127109 [](ancestors = 335127109)
|
||
XamlView SliderViewManager::CreateViewCore(int64_t tag) { | ||
auto slider = winrt::Slider(); | ||
slider.MinHeight(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100 [](start = 19, length = 3)
where 100 comes from? #Resolved
void SliderViewManager::UpdateProperties( | ||
ShadowNodeBase *nodeToUpdate, | ||
const folly::dynamic &reactDiffMap) { | ||
auto slider = nodeToUpdate->GetView().as<winrt::Slider>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as [](start = 40, length = 2)
I guess you means try_as. otherwise as would crash if it's null #Resolved
const folly::dynamic &reactDiffMap) { | ||
auto slider = nodeToUpdate->GetView().as<winrt::Slider>(); | ||
if (slider == nullptr) | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to do this check. #Pending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do, as and try_as return null if the QI fails
In reply to: 335128424 [](ancestors = 335128424)
slider.ClearValue(winrt::Control::IsEnabledProperty()); | ||
} else if (propertyName == "value") { | ||
if (propertyValue.isNumber()) | ||
slider.Value(static_cast<int>(propertyValue.asDouble())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static_cast [](start = 21, length = 16)
slider accept double, and you don't need to cast it #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing two issues:
Microsoft Reviewers: Open in CodeFlow