Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[C, iOS, AND, UWP] TabbedPage SelectedTabColor and UnselectedTabColor #4899

Merged
merged 13 commits into from Mar 1, 2019
Merged

[C, iOS, AND, UWP] TabbedPage SelectedTabColor and UnselectedTabColor #4899

merged 13 commits into from Mar 1, 2019

Conversation

jfversluis
Copy link
Member

@jfversluis jfversluis commented Jan 4, 2019

Description of Change

I opened a previous PR (#3519) for this but that was an iOS-specific one, after the idea of #3478.
In that PR, we got the idea to make it available in general rather than via platform-specifics. This is the PR to do that.

There was already an Android platform-specific for this, I have marked that as obsolete in favor of this.

Issues Resolved

API Changes

Added:

  • Color TabbedPage.UnselectedTabColorProperty { get; set; } //Bindable Property
  • Color TabbedPage.SelectedTabColorProperty { get; set; } //Bindable Property

Platforms Affected

  • Core/XAML (all platforms)
  • iOS
  • Android
  • UWP

Behavioral/Visual Changes

The user is now able to specify a separate color for a selected and unselected tab. There shouldn't be any behavioral changes. Android users of the "old" On<Android>().SetBarItemColor(Color.Red); and On<Android>().SetBarSelectedItemColor(Color.White); will notice that these are now marked as obsolete and are directed to this new method which does the same.

Before/After Screenshots

Android

Before:

Android before

After:

Android after

iOS

Before:

iOS before

After:

iOS after

UWP

Before:

UWP before

After:

UWP after

Testing Procedure

Added a case to the gallery root page RootPages Gallery -> Tab->Content.

Testing on iOS

Original: https://www.dropbox.com/s/4zred5fkizo9y92/SelectedTabColor.mov?dl=0

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

@jfversluis jfversluis changed the title Feature/3478 unselectedtabcolor [C, iOS, AND, UWP] TabbedPage SelectedTabColor and UnselectedTabColor Jan 4, 2019
@jfversluis
Copy link
Member Author

Can I do anything about the failing tests?

@samhouts samhouts added t/enhancement ➕ deprecation Public API has been deprecated e/3 🕒 3 labels Jan 7, 2019
@samhouts samhouts added this to In Review in v3.6.0 Jan 7, 2019
Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@StephaneDelcroix StephaneDelcroix left a comment

Choose a reason for hiding this comment

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

almost there

Xamarin.Forms.Core/TabbedPage.cs Outdated Show resolved Hide resolved
@samhouts samhouts moved this from In Review to In Progress in v3.6.0 Jan 15, 2019
@jfversluis
Copy link
Member Author

Removed last calls to (now) obsolete members in Android which caused the build to fail, sorry about that 🙂

Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

I think we accidentally dropped support for the platform specific. What we can do is make the platform specific just set the real property. This way, we have all new and beautiful code, but the old platform specifics still work.

@jfversluis
Copy link
Member Author

jfversluis commented Jan 24, 2019

@mattleibow I see your point, I'm just not a big fan of multiple ways to do the same thing, that is why I chose to deprecate the one method. Still, for that time I might be able to change the implementation to set the new property. Else we'll have two properties and two ways of doing the same thing. I'm even less of a fan of that 😉

This would also solve the (accidental) dropping the old way. Does that make sense?

Edit: Already made some changes, just awaiting the naming and deprecation then I guess!

Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Looking better. Just had a thought that if we are setting the new property via the old one, we don't actually need to check the old one anymore.

@samhouts samhouts added this to In Progress in v4.0.0 Feb 2, 2019
@samhouts samhouts removed this from In Progress in v3.6.0 Feb 2, 2019
@jfversluis
Copy link
Member Author

Thanks @mattleibow ! Makes sense, I think I got all that you meant

Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

I think we are good.

@samhouts samhouts added the API-change Heads-up to reviewers that this PR may contain an API change label Feb 21, 2019
Co-Authored-By: jfversluis <github@geraldversluis.nl>
@jfversluis
Copy link
Member Author

Suggestion batch committed 😎 thanks @samhouts !

@samhouts samhouts changed the base branch from master to 4.0.0 March 1, 2019 23:46
@samhouts samhouts merged commit e60bfde into xamarin:4.0.0 Mar 1, 2019
v4.0.0 automation moved this from In Progress to Done Mar 1, 2019
@jfversluis jfversluis deleted the feature/3478-unselectedtabcolor branch March 2, 2019 11:17
@samhouts samhouts added this to the 4.0.0 milestone Mar 8, 2019
AxelUser pushed a commit to AxelUser/Xamarin.Forms that referenced this pull request Jun 15, 2019
…xamarin#4899)

* Implemented Core and iOS

* Implemented Android

* Supressed platform specific obsolete warnings for now

* Implemented UWP

* Make selected page on UWP not rely on title

* Review feedback

* Removed references to (now) obsolete members

* Review feedback fixes

* Code review feedback

* Changed obsolete msg to reflect right XF version

* Apply suggestions from code review

Co-Authored-By: jfversluis <github@geraldversluis.nl>
fixes xamarin#3478
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API-change Heads-up to reviewers that this PR may contain an API change deprecation Public API has been deprecated e/3 🕒 3 p/Android p/iOS 🍎 p/UWP t/enhancement ➕
Projects
No open projects
v4.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants