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 colored refresh indicator for ListView pull-to-refresh #2961

Merged
merged 16 commits into from Nov 29, 2018

Conversation

@jfversluis
Copy link
Contributor

jfversluis commented Jun 6, 2018

Description of Change

For a project, I needed to color the spinner which is shown when you pull to refresh on a ListView. To my big surprise, this isn't available by default. So, I thought I would add it since I could find any issue or PR for it.

Implemented for iOS and Android only. For other platforms, I did not see a loading indicator at all, or not one that could be colored.

No tests were added since it's a visual only thing.

Bugs Fixed

  • N/A

API Changes

Added:

public static readonly BindableProperty ListView.RefreshControlColorProperty;
public Color ListView.RefreshControlColor { get; set; }

Behavioral Changes

By default the color of the spinner will be black as is it now, but by setting the new SpinnerColor property you can influence the color of the spinner.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

jfversluis added some commits Jun 6, 2018

@samhouts samhouts added this to In Review in vNext+1 (master) Jun 6, 2018

@@ -46,6 +46,8 @@ public class ListView : ItemsView<Cell>, IListViewController, IElementConfigurat

public static readonly BindableProperty SeparatorColorProperty = BindableProperty.Create("SeparatorColor", typeof(Color), typeof(ListView), Color.Default);

public static readonly BindableProperty SpinnerColorProperty = BindableProperty.Create(nameof(SpinnerColor), typeof(Color), typeof(ListView), Color.Black);

This comment has been minimized.

@rmarinho

rmarinho Jun 7, 2018

Member

I think you should use the Color.Defaultand in the renderers you need to capture the default color of the spinner when they are initialised , so if the user changes back to the default color you can set the default color of the platform.

This comment has been minimized.

@jfversluis

jfversluis Jun 7, 2018

Contributor

I did Color.Default, but then the spinner disappeared for some weird reason, at least on iOS. I'll check it again.

@@ -217,6 +219,12 @@ public Color SeparatorColor
set { SetValue(SeparatorColorProperty, value); }
}

public Color SpinnerColor

This comment has been minimized.

@rmarinho

rmarinho Jun 7, 2018

Member

RefreshControlColor? because it could be something different from a spinner in a specific platform.

This comment has been minimized.

@jfversluis

jfversluis Jun 7, 2018

Contributor

Hm makes sense. I was struggling with the name. RefreshControlColor seems to cover it.

Renamed away from "spinner" and did some default value magic
Color.Default reinstated, but also added the default black color in the iOS `UpdateRefreshControlColor` method. Seeing this also happens for the seperator color this is something that is necessary. If not, the spinner is invisible.
@jfversluis

This comment has been minimized.

Copy link
Contributor

jfversluis commented Jun 7, 2018

@rmarinho updated naming. Color.Default reinstated on the ListView, but also added the default black color in the iOS UpdateRefreshControlColor method. Seeing this also happens for the separator color this is something that is necessary. If not, the spinner is invisible.

@jfversluis jfversluis changed the title Implemented colored spinner for ListView pull-to-refresh Implemented colored refresh indicator for ListView pull-to-refresh Jun 7, 2018

var color = Element.RefreshControlColor;

if (_tableViewController != null)
_tableViewController.UpdateRefreshControlColor(color.ToUIColor(UIColor.Black));

This comment has been minimized.

@rmarinho

rmarinho Jun 7, 2018

Member

Instead of UIColor.Black you need to find someway to find the initial color of the RefreshControl, then on this method when color == Color.Default you call UpdateRefreshControlColor with that color that you capture initially as the default color of the spinner. If you use Color.Default it's transparent so the spinner disappears, you need to capture the native initial color.

This comment has been minimized.

@jfversluis

jfversluis Jun 7, 2018

Contributor

I think I see your point. According to this link, it is nil as a default: https://developer.apple.com/documentation/uikit/uirefreshcontrol/1624847-tintcolor so would that make sense then? Just tested it and set the tint color to null (when the property is Color.Default) makes the spinner black.

I should double check how it is on Android.

jfversluis added some commits Jun 8, 2018

Implemented default color on iOS and right property name on Android
For iOS, when the new `RefreshControlColor` has the default color, the `TintColor` for iOS is set to null, causing the color to be the default one.

Forgot to refactor the new name in the Android renderer, whoops
@jfversluis

This comment has been minimized.

Copy link
Contributor

jfversluis commented Jun 8, 2018

@rmarinho added some more changes for using the default color on iOS

Merge pull request #3 from xamarin/master
Merge from origin
@rmarinho

This comment has been minimized.

Copy link
Member

rmarinho commented Jun 20, 2018

@jfversluis can you rebase please so I can run uitests on this . thanks

@rmarinho rmarinho closed this Jun 20, 2018

vNext+1 (master) automation moved this from In Review to Closed Jun 20, 2018

@rmarinho rmarinho reopened this Jun 20, 2018

vNext+1 (master) automation moved this from Closed to To do Jun 20, 2018

@samhouts samhouts moved this from To do to In Review in vNext+1 (master) Jun 21, 2018

jfversluis added some commits Jun 6, 2018

Renamed away from "spinner" and did some default value magic
Color.Default reinstated, but also added the default black color in the iOS `UpdateRefreshControlColor` method. Seeing this also happens for the seperator color this is something that is necessary. If not, the spinner is invisible.
Implemented default color on iOS and right property name on Android
For iOS, when the new `RefreshControlColor` has the default color, the `TintColor` for iOS is set to null, causing the color to be the default one.

Forgot to refactor the new name in the Android renderer, whoops
@jfversluis

This comment has been minimized.

Copy link
Contributor

jfversluis commented Jun 21, 2018

@rmarinho done and generated documentation for the ListView

@jfversluis

This comment has been minimized.

Copy link
Contributor

jfversluis commented Jul 11, 2018

@rmarinho where are we on this? 🙂

@rookiejava

This comment has been minimized.

Copy link
Collaborator

rookiejava commented Jul 18, 2018

It seems to be limited to android and iOS only. Why don't you use PlatformSpecific?

@jfversluis

This comment has been minimized.

Copy link
Contributor

jfversluis commented Jul 18, 2018

The platform-specifics are usually when something is really tied to one platform. But to be honest I didn't really think about it and thought that I could implement it on more platforms. I still don't rule that out for the future, if only Tizen implemented a loading indicator 😉

Edit: when #2086 gets implemented, UWP can be added to this.

@jfversluis

This comment has been minimized.

Copy link
Contributor

jfversluis commented Sep 21, 2018

How are we here?

@samhouts samhouts moved this from In Review to In Progress in vNext+1 (master) Sep 21, 2018

@samhouts samhouts requested a review from rmarinho Nov 21, 2018

@samhouts

This comment has been minimized.

Copy link
Member

samhouts commented Nov 21, 2018

build --uitests

jfversluis added some commits Nov 22, 2018

@jfversluis

This comment has been minimized.

Copy link
Contributor

jfversluis commented Nov 22, 2018

Resolved merge conflicts with master

@rmarinho

This comment has been minimized.

Copy link
Member

rmarinho commented Nov 22, 2018

build --uitests

@samhouts samhouts added the e/7 🕖 label Nov 27, 2018

@rmarinho rmarinho merged commit 0d08fd8 into xamarin:master Nov 29, 2018

10 checks passed

VSTS: Android API19 Validation Fast Renderers UITests Finished
Details
VSTS: Android API19 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API23 Validation Fast Renderers UITests Finished
Details
VSTS: Android API23 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API25 Validation Fast Renderers UITests Finished
Details
VSTS: Android API25 Validation Legacy Renderers UITests Finished
Details
VSTS: iOS10 Validation UITests Finished
Details
VSTS: iOS11 Validation UITests Finished
Details
Xamarin Forms #PR-2961 - (2229523) succeeded
Details
license/cla All CLA requirements met.
Details

vNext+1 (master) automation moved this from In Progress to Done Nov 29, 2018

@jfversluis jfversluis deleted the jfversluis:feature/coloredpulltorefresh branch Nov 29, 2018

@samhouts samhouts added the approved label Nov 29, 2018

@samhouts samhouts added this to the 4.0.0 milestone Dec 4, 2018

@samhouts samhouts removed this from Done in vNext+1 (master) Jan 3, 2019

@samhouts samhouts modified the milestones: 4.0.0, 3.5.0 Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment