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

Regular Polygon feature #32

Merged
merged 24 commits into from Aug 12, 2019

Conversation

@shanranm
Copy link
Contributor

commented Aug 9, 2019

Addresses #31

@sthewissen

This comment has been minimized.

Copy link
Owner

commented Aug 9, 2019

I'm not a fan of the IsRegular property. I think a cleaner solution is making Sides a default value of 4. Wherever IsRegular is used would then become a check for Sides being 4 or not.

Also perhaps an additional check needs to be added that throws an ArgumentException when sides is not 3 or more. There's already a similar ArgumentException for background gradient angle which needs to be 0-360. Does this also apply for your OffsetAngle?

@shanranm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

As I've mentioned on the issue, sides by default is 4, but IsRegular switches it from begin a Rectangle to regular polygon - Square
Will add the checks on Sides. OffsetAngle, well its an angle, can be anything, above 360 means - value % 360. Will add checks just to be in consistent with gradient angle.

@sthewissen

This comment has been minimized.

Copy link
Owner

commented Aug 9, 2019

Yeah, I saw the comments you added there only after I posted mine. Wups. I will try to take a look at this branch today!

@sthewissen

This comment has been minimized.

Copy link
Owner

commented Aug 9, 2019

One of the things I've been struggling with lately is figuring out a way to check whether or not all of the supported properties work and work when combined with one another. Adding polygons adds a whole new dimension to that as well. A lot of scenarios to support. The DebugPage was a start for that, but maintaining that manually becomes a chore. Will look into something coming up with something for this myself.

@shanranm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

I was also thinking on the same lines.. hence the new Polygon try out page. Still needs a lot of work in adding toggle for all the features...

@sthewissen

This comment has been minimized.

Copy link
Owner

commented Aug 9, 2019

Yeah, I really like that idea. Will try to expand upon that too for the normal pancakes.

@sthewissen

This comment has been minimized.

Copy link
Owner

commented Aug 9, 2019

Do you mind if I start expanding on your idea for the debugging page for the "normal" PancakeView? Perhaps we can at some point merge them into one big debugging page with all the options?

image

@shanranm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Sure, please go ahead!
Love the monkey face!!

@sthewissen

This comment has been minimized.

Copy link
Owner

commented Aug 9, 2019

If all is well, this new debug page should now be feature complete. Some properties aren't working, which is nice to see, because that tells me where I/we still need to do some work 😂

image

@shanranm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Wow, now THIS is a Debug Page!
I can see lots of things that are not supposed to happen 😄
Just to be safe I reverted back to master and opened this page, some things are real messed up!! Especially with gradients. I guess no one's changing gradient at runtime.
We need to fix them before even thinking of merging this PR 😞

@sthewissen

This comment has been minimized.

Copy link
Owner

commented Aug 9, 2019

I've been testing on iOS and I think quite a bit of things are working better than I had actually expected 😅That'll probably be my first focus, since I know the iOS code better.

sthewissen added 3 commits Aug 9, 2019
@sthewissen

This comment has been minimized.

Copy link
Owner

commented Aug 10, 2019

@shanranm - I fixed almost all the iOS rendering bits. The only scenario that is still giving problems right now is the non-square + gradient border + small corner radius at some rotations. Gradient borders were a tough nut to crack and I've had to give up on BorderDrawingStyle for now.

image

shanranm added 2 commits Aug 10, 2019
@shanranm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

And that sir, should fix everything.
well, of course leaving out the BorderDrawingStyle

@sthewissen

This comment has been minimized.

Copy link
Owner

commented Aug 11, 2019

You mean fix everything on both platforms or just iOS? I just tested iOS and that works like a charm! Thanks for all the hard work so far, I really appreciate it. Couldn't have done it without you 😃

Moving on to Droid; Running the debug app on Android on a GenyMotion emulator I get some really weird behavior. Not sure if that's a good test case though, but I don't have my physical Android device near. All the sliders in there are also kind of behaving weirdly 😂 Just some of the things I see:

  • BackgroundGradient not working.
  • Dashed border behaving weirdly.
  • Clipping + border produces no border.
  • Sides/offset angle seem to misbehave.

image

@shanranm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

I swear I fixed on Android too 😄
May be dependent on Android version?
I did debug on a physical device with version - 8.1.0
Still, will take a look again

@sthewissen

This comment has been minimized.

Copy link
Owner

commented Aug 11, 2019

Yeah, like I said, I'm not sure how good my test setup is for this. I'll have access to a physical device tomorrow, so will also recheck then. This was an 8.0 (API26) emulator.

@shanranm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

I was able to reproduce all of them on an emulator.
For some weird reason, Draw is not called on PancakeDrawable when ONLY BackgroundColor is changed. To handle this, new drawable is created (also disposing the older one)

shanranm and others added 2 commits Aug 11, 2019
@sthewissen

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2019

Cool, that last update fixes a lot! Is it just me though or are these sliders rendering very weirdly and very un-userfriendly (no track, very hard to control)?

image

Also, seems like the background bleeds out from under the border in some cases?

image

I also just fixed a small bug where there would be a minuscule border when it's set to 0.

@shanranm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Yea, sliders have issue on Android, had to manually set WidthRequest for debugging.
Just set it as style, will be applied to all elements on the page.
The background leaking have always been there for sides = 4... i'd like to rewrite another create path specifically for sides == 4 case... may be another PR?

@sthewissen

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2019

Agreed. Let's merge this one!

Once again though, let me make clear that I really appreciate what you've done in this PR! It truly is an awesome addition 😃

@sthewissen sthewissen merged commit 3c009b2 into sthewissen:master Aug 12, 2019

1 check passed

PancakeView-CI #485 succeeded
Details
@shanranm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

It was my pleasure!! 😄
I don't think it would've been come this far without your active and continuous support!!

@shanranm shanranm deleted the shanranm:polygon branch Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.