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

Added the StateLayout #450

Merged
merged 17 commits into from Oct 30, 2020
Merged

Added the StateLayout #450

merged 17 commits into from Oct 30, 2020

Conversation

sthewissen
Copy link
Contributor

@sthewissen sthewissen commented Oct 16, 2020

Description of Change

This PR adds StateLayout and all of its helper classes. This is currently up as a separate NuGet called "StateSquid" here:
https://www.nuget.org/packages/Xamarin.Forms.StateSquid 🦑

Bugs Fixed

API Changes

Added all the classes as already described in #275. The only thing I haven't put in here is the SkeletonView that was mentioned there. I don't want to take that in to be honest, as I think it should be up to the user to create their own visualization. I'm sure we could come up with a better skeleton loading solution anyway.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Updated documentation

@sthewissen
Copy link
Contributor Author

I hope to be able to add some documentation for this and can port some of the existing samples from the repo. Not sure if I'll come around to writing some adequate tests due to time constraints, so if anyone is up for that feel free to add them.

@jfversluis
Copy link
Member

Builds 🎉

Success,
Error,
Empty,
Custom
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder, is there any sense to keep enum if we cannot describe all possible cases?

Maybe we can refactor it to static class (similar to how XF Device class operates with RuntimePlatform)

public static class LayoutState {
    public const string None = nameof(None);
    public const string Loading = nameof(Loading);
    /// etc.
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Been trying this but so far it's been a bunch of misery :P The custom state is explicitly added for scenarios not covered by the default states. Also making it a string introduces some strange scenarios where the user has to explicitly set a value for it to work. We can fix that in the component obviously, but it feels a bit strange.

@@ -0,0 +1,13 @@
namespace Xamarin.CommunityToolkit.UI.Views
{
public enum State
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, LayoutState?

State is too general, I believe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thing is, that would mean we have StateLayout and LayoutState. That confuses me even more 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's fair. But if we will add several new controls which have "State" property, it will confuse not less :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Perhaps I shall think a bit longer on an alternative :)

Copy link
Member

Choose a reason for hiding this comment

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

Are we still going to change this name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to suggestions :)

@AndreiMisiukevich
Copy link
Contributor

Hmmm, something went wrong with rebase, I guess

@sthewissen
Copy link
Contributor Author

Hmmm, something went wrong with rebase, I guess

Working on it :)

@sthewissen
Copy link
Contributor Author

sthewissen commented Oct 29, 2020

I rebased once more. Unless we can come up with a better name I'd say leave State as is for now. ViewState is a possible alternative, but that can also be a bit confusing. Regarding making the State constant strings, I'm personally not a fan as it would require quite rewriting and logic adjustments. I personally don't have the time for that and would make migration for existing users more cumbersome (I think). @jfversluis @AndreiMisiukevich

@AndreiMisiukevich
Copy link
Contributor

I rebased once more. Unless we can come up with a better name I'd say leave State as is for now. ViewState is a possible alternative, but that can also be a bit confusing. Regarding making the State constant strings, I'm personally not a fan as it would require quite rewriting and logic adjustments. I personally don't have the time for that and would make migration for existing users more cumbersome (I think). @jfversluis @AndreiMisiukevich

That's up to you.
But I do like "LayoutState" name. Similar to "ExpandState" in Expander. Or StateLayoutState.

@sthewissen
Copy link
Contributor Author

@AndreiMisiukevich I renamed to LayoutState, let's see how it goes :)

@sthewissen sthewissen merged commit 2f43c15 into main Oct 30, 2020
@sthewissen sthewissen deleted the statelayout branch October 30, 2020 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants