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

[Material] [Android, iOS] Materializing the stepper #5027

Merged
merged 16 commits into from Feb 22, 2019

Conversation

mattleibow
Copy link
Contributor

@mattleibow mattleibow commented Jan 19, 2019

Description of Change

Adding a renderer for the material stepper:

  • refactored the actual stepper logic out so that all the controls can just implement it
  • added a new renderer for material

Issues Resolved

API Changes

  • a new MaterialStepperRenderer
  • a new StepperRendererManager to manage all stepper logic

Platforms Affected

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

Behavioral/Visual Changes

  • Changed to more visibility pleasing + and -
  • Due to the text values being changed to a different version of +/- if a UI test is using app.Text("+") to locate the button it won't find it but if it's using app.Marked or nothing it will be able to find it just fine

Before/After Screenshots

Material (with full-width characters / new characters)

Android iOS

Default (with full-width characters)

Android iOS

Material (with plus and hyphen / old characters)

Android iOS

Testing Procedure

Should just be a visual style change when using Material. Functionality should be exactly the same.

PR Checklist

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

@mattleibow
Copy link
Contributor Author

When testing the tabstops fix in #5000 (which I have included here because I modified the renderer code), I noticed that the Material stepper doesn't seem to be working with tabs. I am not yet sure what the cause may be, but I do see a lot of extra code in the base renderers that we may need to re-implement.

@mattleibow
Copy link
Contributor Author

I reverted the changes to use a "Fast" renderer as it doesn't seem to work with multiple children views. I probably need to investigate this further, but for now steppers aren't the most important control.

@mattleibow
Copy link
Contributor Author

To test the tab stops on Android, we also need #4612.

@samhouts samhouts added this to In Review in v3.6.0 Jan 19, 2019
@samhouts samhouts added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Jan 19, 2019
@samhouts samhouts added this to Ready for Review (PRs) in Sprint 147 Jan 19, 2019
@mattleibow
Copy link
Contributor Author

Thanks for the feedback, will get on that. In the meantime, I added the iOS one. Not sure if creating a new "control" is cool for iOS - or where I should put it.

This is not exactly what I wanted to do, since there is no need for the control - I can just do the layout in the renderer. But, I can't seem to find the concept of a no-native-view renderer. I am also thinking that this might also be good for Android.

@samhouts samhouts added this to Ready for Review (PRs) in Sprint 148 Jan 22, 2019
}
}

public class MaterialStepper : UIView
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 sure where we want to put this - this is very this-renderer specific and has no use elsewhere. Basically - just for layout.

@mattleibow mattleibow changed the title [WIP] [Material] Materializing the stepper [Material] [Android, iOS] Materializing the stepper Jan 22, 2019
@mattleibow
Copy link
Contributor Author

This comment is still up for discussion:
#5027 (review)

@PureWeen PureWeen removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Jan 25, 2019
@samhouts samhouts added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Jan 26, 2019
@PureWeen PureWeen removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Jan 28, 2019
@samhouts samhouts added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Jan 29, 2019
Copy link
Contributor

@hartez hartez left a comment

Choose a reason for hiding this comment

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

Looks good, just have a few questions.

Sprint 147 automation moved this from Ready for Review (PRs) to In progress Jan 29, 2019
Sprint 148 automation moved this from Ready for Review (PRs) to In progress Jan 29, 2019
@samhouts samhouts moved this from In Review to In Progress in v3.6.0 Jan 29, 2019
@samhouts samhouts assigned paymicro and unassigned hartez Feb 20, 2019
@PureWeen PureWeen added the blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. label Feb 21, 2019
@PureWeen PureWeen merged commit c4df5b2 into 3.6.0 Feb 22, 2019
v3.6.0 automation moved this from In Progress to Done Feb 22, 2019
Sprint 149 automation moved this from In progress to Done Feb 22, 2019
@PureWeen PureWeen added the breaking Changes behavior or appearance label Feb 22, 2019
@PureWeen PureWeen deleted the mattleibow/material-stepper branch February 22, 2019 19:53
@samhouts samhouts added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Mar 2, 2019
@samhouts samhouts added this to the 3.6.0 milestone Mar 5, 2019
@samhouts samhouts added the approved Has two approvals, no pending reviews, and no changes requested label Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a/visual approved Has two approvals, no pending reviews, and no changes requested blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. breaking Changes behavior or appearance p/Android p/iOS 🍎 retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. t/bug 🐛 t/enhancement ➕
Projects
No open projects
Sprint 148
  
In progress
Sprint 149
  
Done
v3.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants