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-18040] - Android: expose gravity constants and add documentation #6435

Merged
merged 8 commits into from Apr 25, 2016

Conversation

manumaticx
Copy link
Contributor

@manumaticx manumaticx changed the title [TIMOB-18040] - Android gravity constants and docs [TIMOB-18040] - Android: expose gravity constants and add documentation Dec 7, 2014
@hansemannn
Copy link
Collaborator

Hi @manumaticx, can you update this branch to work with the latest master? Would be great to have it in for 6.0.0! /cc @ashcoding

# Conflicts:
#	android/modules/ui/src/java/ti/modules/titanium/ui/android/AndroidModule.java
@manumaticx
Copy link
Contributor Author

@hansemannn All's well.

@hansemannn
Copy link
Collaborator

Great! I also forgot to say, that you might change the docs version from "3.6.0" to "6.0.0" as part of the upcoming release.

@manumaticx
Copy link
Contributor Author

@hansemannn sry, I missed that. Updated as well

@ashcoding
Copy link
Contributor

Code review looks good. Will have a look at functionally testing this soon.

summary: Raw bit controlling whether the right/bottom edge is clipped to its container, based on the gravity direction being applied.
type: Number
permission: read-only
since: "6.0.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "since" is for the Ti-version, not the Android version. Can you correct all constants based on your current knowledge? You could make a "description" param that says something like "Only available on Android 6 and later". If some are available earlier / later, go ahead adjust it.

@hansemannn
Copy link
Collaborator

@manumaticx Left some additional comments. Can you attach a simple test case that can be thrown in an app.js?

@manumaticx
Copy link
Contributor Author

Okay, PR updated. Here is code for testing this:

function gravityTest(){

  var gravityConstants = [
    "GRAVITY_AXIS_CLIP",
    "GRAVITY_AXIS_PULL_AFTER",
    "GRAVITY_AXIS_PULL_BEFORE",
    "GRAVITY_AXIS_SPECIFIED",
    "GRAVITY_AXIS_X_SHIFT",
    "GRAVITY_AXIS_Y_SHIFT",
    "GRAVITY_BOTTOM",
    "GRAVITY_CENTER",
    "GRAVITY_CENTER_HORIZONTAL",
    "GRAVITY_CENTER_VERTICAL",
    "GRAVITY_CLIP_HORIZONTAL",
    "GRAVITY_CLIP_VERTICAL",
    "GRAVITY_DISPLAY_CLIP_HORIZONTAL",
    "GRAVITY_DISPLAY_CLIP_VERTICAL",
    "GRAVITY_END",
    "GRAVITY_FILL",
    "GRAVITY_FILL_HORIZONTAL",
    "GRAVITY_FILL_VERTICAL",
    "GRAVITY_HORIZONTAL_GRAVITY_MASK",
    "GRAVITY_LEFT",
    "GRAVITY_NO_GRAVITY",
    "GRAVITY_RELATIVE_HORIZONTAL_GRAVITY_MASK",
    "GRAVITY_RELATIVE_LAYOUT_DIRECTION",
    "GRAVITY_RIGHT",
    "GRAVITY_START",
    "GRAVITY_TOP",
    "GRAVITY_VERTICAL_GRAVITY_MASK"
  ];

  gravityConstants.forEach(function(gravity){
    Ti.UI.createNotification({
        message: gravity,
        duration: Ti.UI.NOTIFICATION_DURATION_SHORT,
        gravity: Ti.UI.Android[gravity]
    }).show();
  });
}

@hansemannn
Copy link
Collaborator

Thanks, will test today! One thing I forgot: Can you add platforms: [android] to the new property docs to ensure it's Android-only.

@manumaticx
Copy link
Contributor Author

@hansemannn is this necessary?
platforms: [android] is already defined for the Ti.UI.Android namespace which should apply to all properties within that.

@hansemannn
Copy link
Collaborator

@manumaticx: Not necessary, just nice to have for advanced API-clarification. It's fine to skip it for now and we'll add in a future docs updates. The reason is, that the constants are clearly Android, but the "gravity" property is part of the Ti.UI.Notification API which is cross-platform.

@manumaticx
Copy link
Contributor Author

Ah, the gravity proprerty itself. That's right, let me add this quickly. I forgot that (back when I submitted this PR Ti.UI.Notification was Android only)

@manumaticx
Copy link
Contributor Author

I guess I have to squash these commits now!? 😊

@hansemannn
Copy link
Collaborator

You can create an additional commit to fix the line-breaks, thats fine. EDIT: Wrong PR, but yeah you can leave it like it is. Will merge as soon as travis finishes.

summary: Determines the location at which the notification should appear on the screen.
type: Number
constants: Titanium.UI.Android.GRAVITY_*
platforms: [android]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @manumaticx, forgot to finish the review here. Please add the since: "5.4.0" here as well and we can merge today. Thank you! 🚀

@hansemannn
Copy link
Collaborator

Approved!

@hansemannn hansemannn merged commit b466697 into tidev:master Apr 25, 2016
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

3 participants