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

TIDOC-623 TIDOC-622 APIDOC: Ti.UI.TableViewSection issues #2253

Merged
merged 4 commits into from
Jun 11, 2012

Conversation

hal-gh
Copy link
Contributor

@hal-gh hal-gh commented May 23, 2012

extends: Titanium.UI.View
excludes: {
methods: [ animate ],
properties: [ anchorPoint, animatedCenterPoint, backgroundDisabledColor, backgroundDisabledImage,
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of these props and the animate method are supported on sections in MW, but is this something we want to communicate in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you able to say which ones should stay in this excludes directive?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just anchorPoint and animatedCenterPoint, although that's just because MW doesn't support them at all. I guess, technically speaking, they all should stay here and let the documentation for anchorPoint and animatedCenterPoint indicate that MW doesn't support them?

@mstepanov
Copy link
Contributor

Looks good from iOS perspective.

borderRadius, borderWidth, bottom, focusable, height, layout, left, right, rect,
size, top, touchEnabled, transform, visible, width, zIndex
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Many of these props are not supported in Android.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pingwang2011 this is excludes section

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I did not notice this is excludes section. Please ignore my previous comment.

@pingwang2011
Copy link
Contributor

doc reviewed and ran validate.py and docgen.py. passed. Accepted

@hal-gh
Copy link
Contributor Author

hal-gh commented May 31, 2012

Arthur - I need assistance with this. Please would you ping me on IRC?
Thanks

@arthurevans
Copy link
Contributor

Since this isn't a view proxy at all on iOS, I don't think we want to encourage people to treat it like one on other platforms unless there's a compelling reason to do so.

So I would lean towards keeping the "excludes" directive as is unless Bryan thinks there's a good case for using these methods/props on MW.

@hal-gh
Copy link
Contributor Author

hal-gh commented May 31, 2012

Bryan, please let me know if there is any other action I need to take in order for this PR to be ready to merge.
Thanks

@nebrius
Copy link
Contributor

nebrius commented May 31, 2012

I still need to re-review it. At a conference right now. Hopefully will get to it tomorrow.

@nebrius
Copy link
Contributor

nebrius commented Jun 1, 2012

I think we should do one of two things going forward:

  1. Treat this as a view proxy like it is on Android and a view on Mobile Web (we don't have proxies). We would need to file a parity ticket against iOS in this case to make their tableviewsections view proxies.

  2. Change the documentation for tableviewsection to say that it does not inherit from Ti.UI.View. After thinking about it, saying that this control inherits from view but doesn't implement any of it is kinda silly.

I would prefer 1), but I recognize that 2) is much more feasible.

@arthurevans
Copy link
Contributor

As near as I can tell (iOS folks, feel free to jump in here), the section isn't even an object on iOS, let alone a view--it's just an integer ID value. You can set a header for section N, set a footer for section N, add a cell to section N--but any data about the section itself appears to be internal to the TableView object.

If that's correct, then yes--2 is probably going to be more feasible.

@mstepanov
Copy link
Contributor

@arthurevans you are correct on table sections in iOS

@arthurevans
Copy link
Contributor

OK, we're going to change the inheritance form Ti.UI.View to Ti.Proxy.

@arthurevans
Copy link
Contributor

Paul, if you're out there, let me know ;-). Otherwise, I'll finish this one off. Thanks.

@hal-gh
Copy link
Contributor Author

hal-gh commented Jun 6, 2012

Hi Arthur - I was trying to catch you online a few days ago to discuss this, but you weren't around.
I will ping you on IRC about it.
Thanks

@arthurevans
Copy link
Contributor

Updated, please re-review.

Also: I'm a little unsure about the git provenance of pushing to Paul's repo. It seems like there should be more stuff in the "Merge branch master" since paul's PR was 6 days old but, uh... OK, I'm kind of lost in the swervy, curvy logic of git here. If anyone thinks I should cherry-pick these to a clean branch and start a new PR, I can do that instead.

(that is, an object that inherits from [View](Titanium.UI.View)). However, this is an
implementation detail. For portability, you should only use the documented APIs on
table view sections.
extends: Titanium.Proxy
Copy link
Contributor

Choose a reason for hiding this comment

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

Mobile Web doesn't have proxies, so this doesn't apply to us.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, from a doc point of view, all Ti objects inherit from Ti.Proxy, which defines the methods addEventListener and removeEventListener. All Ti types have to inherit from Ti.Proxy as far as the doc system is concerned. I think the MW equivalent is Evented or something like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok, yeah our equivalent is Evented. Why is it called Proxy? That's not very intuitive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the doc format was developed before there was a Mobile Web. And on Android and iOS, all titanium types extend a class called Proxy, which has certain characteristics that set it apart from normal JS objects.

So, FWIW, I find the MobileWeb equivalent not very intuitive ;-).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see...I don't like it (what does "proxy" have to do with "events", which are the only methods exposed), but if the object doesn't behave like a normal JS object, then I guess it's worth documenting. The documentation for Proxy needs to be updated or created to mention the differences between Mobile Web and the other two platforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't want to put more into Paul's pull request, but I've opened a new ticket to supplement the existing doc for Ti.Proxy.
https://jira.appcelerator.org/browse/TIDOC-668

@nebrius
Copy link
Contributor

nebrius commented Jun 9, 2012

Docs reviewed for Mobile Web. Request accepted.

@pingwang2011
Copy link
Contributor

Doc reviewed for Android. Accepted

pingwang2011 added a commit that referenced this pull request Jun 11, 2012
TIDOC-623 TIDOC-622 APIDOC: Ti.UI.TableViewSection issues
@pingwang2011 pingwang2011 merged commit 3783942 into tidev:master Jun 11, 2012
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

5 participants