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

IndicatorView IndicatorTemplate Binding #19004

Merged

Conversation

VladislavAntonyuk
Copy link
Contributor

@VladislavAntonyuk VladislavAntonyuk commented Nov 24, 2023

Description of Change

Set BindingContext for IndicatorTemplate view

Issues Fixed

Fixes xamarin/Xamarin.Forms#9597, xamarin/Xamarin.Forms#10117

Before After
image image

@VladislavAntonyuk VladislavAntonyuk requested a review from a team as a code owner November 24, 2023 09:36
@ghost ghost added the community ✨ Community Contribution label Nov 24, 2023
@ghost
Copy link

ghost commented Nov 24, 2023

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

@@ -139,6 +141,7 @@ void AddExtraIndicatorItems()
HeightRequest = size,
CornerRadius = _indicatorView.IndicatorsShape == IndicatorShape.Circle ? (float)size / 2 : 0
};
indicator.BindingContext = items[i];
Copy link
Member

Choose a reason for hiding this comment

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

Can we add the indicators as logical children to the indicatorview?

Copy link
Contributor Author

@VladislavAntonyuk VladislavAntonyuk Nov 26, 2023

Choose a reason for hiding this comment

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

do you mean call _indicatorView.AddLogicalChild(indicator);?

Copy link
Member

Choose a reason for hiding this comment

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

Apologies on the late response. Looking closer I realize that suggestion isn't correct.

Out of curiosity, do you think this code could get slightly reworked to just use BindableLayout? This class feels like it's kind of just re-implementing BindableLayout.

I worry about other issues cropping up inside the IndicatorStackLayout that have already been considered by BindableLayout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not familiar with BindableLayout. I will take a look if we can replace the layout.

Copy link
Member

Choose a reason for hiding this comment

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

Yes it could @PureWeen

Copy link
Member

Choose a reason for hiding this comment

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

Maybe for net9 ?

@jfversluis jfversluis requested a review from PureWeen December 4, 2023 14:21
@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Jan 8, 2024
@tranb3r
Copy link

tranb3r commented Jan 17, 2024

@VladislavAntonyuk @PureWeen
What is blocking this PR? Thanks

@mattleibow
Copy link
Member

/rebase

@mattleibow
Copy link
Member

@VladislavAntonyuk could you rebase so we can get another CI build?

@VladislavAntonyuk VladislavAntonyuk force-pushed the indicatorview-itemtemplate-binding branch from f5d6ea8 to 5eab0e2 Compare January 19, 2024 14:11
@VladislavAntonyuk
Copy link
Contributor Author

@VladislavAntonyuk could you rebase so we can get another CI build?

@mattleibow done

@tranb3r
Copy link

tranb3r commented Feb 28, 2024

Is it possible to merge this PR asap?
It's required for the solution @VladislavAntonyuk suggested months ago for a TabView control.
Thanks.

@tranb3r
Copy link

tranb3r commented Mar 7, 2024

@PureWeen Is it possible to merge this PR in SR3?
It's required for the solution @VladislavAntonyuk suggested months ago for a TabView control in Maui CommunityToolkit.
Thanks.

@rmarinho
Copy link
Member

/azp run MAUI-UITests-public

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

rmarinho
rmarinho previously approved these changes Mar 18, 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.

There are build errors:
D:\a\_work\1\s\src\Controls\src\Core\IndicatorView\IndicatorStackLayout.cs(159,38): error CS0103: The name '_indicatorViewPropertyChanged' does not exist in the current context [D:\a\_work\1\s\src\Controls\src\Core\Controls.Core.csproj::TargetFramework=net8.0-android]

@PureWeen
Copy link
Member

PureWeen commented Apr 5, 2024

/rebase

@github-actions github-actions bot force-pushed the indicatorview-itemtemplate-binding branch from 3189a45 to 05aaf1d Compare April 5, 2024 19:14
@PureWeen
Copy link
Member

PureWeen commented Apr 5, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen dismissed jsuarezruiz’s stale review April 5, 2024 21:48

Build errors fixed

@PureWeen
Copy link
Member

PureWeen commented Apr 5, 2024

Failing test on core is unrelated

@PureWeen PureWeen merged commit 6f5ca12 into dotnet:main Apr 5, 2024
45 of 47 checks passed
@VladislavAntonyuk VladislavAntonyuk deleted the indicatorview-itemtemplate-binding branch April 6, 2024 18:28
@github-actions github-actions bot locked and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView community ✨ Community Contribution fixed-in-8.0.20 fixed-in-9.0.0-preview.4.10690 stale Indicates a stale issue/pr and will be closed soon
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Enhancement] Make IndicatorView pass the current item for it's DataTemplate
7 participants