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

Implemented ios:Slider.UpdateOnTap #20124

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kubaflo
Copy link
Contributor

@kubaflo kubaflo commented Jan 24, 2024

Description of Change

I implemented the ios:Slider.UpdateOnTap property in the same way it has been implemented in Xamarin Forms

Issues Fixed

Fixes #20098

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-01-24.at.01.02.02.mp4

@kubaflo kubaflo requested a review from a team as a code owner January 24, 2024 00:04
@ghost ghost added the community ✨ Community Contribution label Jan 24, 2024
@ghost
Copy link

ghost commented Jan 24, 2024

Hey there @kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@samhouts samhouts added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Jan 25, 2024
Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

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

Could you include a sample to allow check/test the platform specific here: https://github.com/dotnet/maui/tree/main/src/Controls/samples/Controls.Sample/Pages/PlatformSpecifics/iOS?

@kubaflo
Copy link
Contributor Author

kubaflo commented Jan 26, 2024

@jsuarezruiz sure I will do it later today

@kubaflo
Copy link
Contributor Author

kubaflo commented Jan 27, 2024

@Eilon Eilon added area-controls-slider Slider and removed legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor labels May 13, 2024
@PureWeen
Copy link
Member

PureWeen commented Jun 2, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

jsuarezruiz
jsuarezruiz previously approved these changes Jun 11, 2024
@Yves-Be
Copy link

Yves-Be commented Jun 19, 2024

This seems a bit wonky when using a centered Slider and a fixed width, I'm getting negative values that are being set.
<Slider Minimum="0" Maximum="100" WidthRequest="256" HeightRequest="64" HorizontalOptions="Center"/>

Perhaps something is still missing compared to Xamarin? tappedLocation.X now seems to be relative to the control frame already, so I don't think we need to subtract control.Frame.X

@kubaflo
Copy link
Contributor Author

kubaflo commented Jun 24, 2024

@Yves-Be you're absolutely right! Thank you yot pointing it out!

@kubaflo kubaflo self-assigned this Jun 24, 2024
@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

{
if (_sliderTapRecognizer == null)
{
_sliderTapRecognizer = new UITapGestureRecognizer((recognizer) =>
Copy link
Member

Choose a reason for hiding this comment

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

This is capturing the handler and thus the platform view and virtual view (which also holds on to the handler) creating a circular reference I think. @jonathanpeppers?

Maybe we need to add a specific memory leak test for this?

Or, maybe we need another proxy type situation to avoid even doing anything remotely scary of keeping the universe alive.

Might just be able to add this into the slider proxy that already exists...

@@ -30,6 +31,38 @@ protected override void DisconnectHandler(UISlider platformView)
_proxy.Disconnect(platformView);
}

internal void MapUpdateOnTap(bool isMapUpdateOnTapEnabled)
Copy link
Member

Choose a reason for hiding this comment

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

@PureWeen do we have "design docs" on where this should live? For other Controls-only things we just kept it in the XAML control class?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, if we're not adding an interface at the core level then controls level inside the xaml class makes sense

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.

iOS-specific Slider.UpdateOnTab doesn't work
8 participants