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

[TIMOB-25775] iOS: Fix gradient getters #9819

Merged
merged 10 commits into from May 18, 2018
Merged

Conversation

hansemannn
Copy link
Collaborator

JIRA: https://jira.appcelerator.org/browse/TIMOB-25775

Eventually to be merged before #9815

@build
Copy link
Contributor

build commented Feb 13, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@hansemannn
Copy link
Collaborator Author

@sgtcoolguy We now lint the unit-tests as well? Thats cool. But do we have a CLI command to fix it as well?

@build build added the ios label Feb 13, 2018
@jquick-axway
Copy link
Contributor

Question:
Do you need to do the same for backfillStart and backfillEnd?

@hansemannn
Copy link
Collaborator Author

@jquick-axway You mean the getters? There are supposed to work already, but the PR is still work in progress to investigate that.

@hansemannn
Copy link
Collaborator Author

@jquick-axway Updated the tests and cleaned up the commit mess. Your decision if 7.1.0 or not :-).

@hansemannn hansemannn modified the milestones: 7.1.0, 7.2.0 Feb 16, 2018
@build build added the ios label Feb 16, 2018
y: 50
},
colors: [ 'red', 'blue' ],
startRadius: '90%',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we officially support percentage based radius settings? We currently document it as "Number" and not "Number/String".
http://docs.appcelerator.com/platform/latest/#!/api/Gradient-property-startRadius

I didn't implement support for this on Android because I assumed we didn't support this on iOS. It would be nice to have though, but the question is, what's it relative to? The width? The height? The shortest dimension? The longest dimension?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do support it because iOS handles all units in a central class (TiDimension). So we could support it on other platforms as well, but later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, from looking at our iOS code here...
https://github.com/hansemannn/titanium_mobile/blob/8261c1c17d11628c9fe4b0a9452905e378fb254b/iphone/Classes/TiGradient.m#L244

A percentage based radius is relative to the distance from the center of the view to one of its corners (ie: hypotenuse divided by 2). Good to know.

I'm trying to provide an option for devs to draw a circle ring without having to use our "borderRadius" feature. So, we might want to make what the percentage based radius is relative to settable in the future so that the circle won't get clipped.


- (NSNumber *)startRadius
{
return [self valueForUndefinedKey:@"startRadius"];
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't document percentage based radius settings, but this will return it as a number between 0-100, right? I think it should return the original percentage string.

If we don't officially support percentage based radius, then I'm okay for making this a task for later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See comment above. But you are right, I need to change the return type to id.

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

@build build added the ios label Apr 23, 2018
@sgtcoolguy sgtcoolguy modified the milestones: 7.2.0, 7.3.0 May 16, 2018
@ssjsamir ssjsamir self-requested a review May 16, 2018 20:20
Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed: Able to receive a log of the gradient properties.

Test steps

Test Environment
APPC Studio: 5.0.0.201712081732
APPC CLI: 7.0.3
iphone 6 plus (10.2)
Operating System Name: Mac OS High Sierra
Operating System Version: 10.13
Node.js Version: 8.9.1
Xcode 9.2

@hansemannn
Copy link
Collaborator Author

@jquick-axway @ssjsamir I've fixed the unit-test linting manually now. Still wondering if there is a better way.

@hansemannn hansemannn merged commit 935d5c5 into tidev:master May 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants