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

docs: update Ti.Android.R and Ti.App.Android.R docs #11492

Merged
merged 4 commits into from
Mar 5, 2020

Conversation

drauggres
Copy link
Contributor

In docs before this PR:

  1. R described as both: R property of Ti.Android and class R (in Ti.Android namespase) that extends Ti.Proxy
  2. Same for R in Ti.App.Android
  3. Documentation for Ti.App.Android.R was almost empty.

This PR should fix these problems.

Required for correct fix for tidev/docs-devkit#29.

@build build added this to the 9.1.0 milestone Feb 21, 2020
@build build requested a review from a team February 21, 2020 08:39
@build
Copy link
Contributor

build commented Feb 21, 2020

Warnings
⚠️ There is no linked JIRA ticket in the PR body. Please include the URL of the relevant JIRA ticket. If you need to, you may file a ticket on JIRA
Messages
📖

💾 Here's the generated SDK zipfile.

📖 🎉 Another contribution from our awesome community member, drauggres! Thanks again for helping us make Titanium SDK better. 👍
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 6507 tests are passing.
(There are 703 skipped tests not included in that total)

Generated by 🚫 dangerJS against 161b012

Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

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

Left a few notes on some phrasing. I would also like keep the type defined as Titanium.Android.R instead of renaming it to RProxy. See my comment in the the related PR on the docs-devkit how we could handle that.

apidoc/Titanium/App/Android/Android.yml Outdated Show resolved Hide resolved
apidoc/common/RProxy.yml Outdated Show resolved Hide resolved
apidoc/Titanium/App/Android/Android.yml Outdated Show resolved Hide resolved
apidoc/Titanium/Android/Android.yml Outdated Show resolved Hide resolved
@drauggres
Copy link
Contributor Author

@janvennemann Thanks for review.
There are two problems in the generated .d.ts:

  1. Properties of the Ti.Android.R are defined as properties of the instance (not static), so they are not available (e.g. Ti.Android.R.drawable will not work).
  2. Ti.App.Android.R don't have any properties (except inherited from Ti.Proxy)

First problem could be solved in the generator solely. But the second couldn't, we need to change the docs.
Currently in docs there are 4 Rs: 2 constants and 2 classes in Ti.App.Android and Ti.Android namespaces, but in the code there are only 3 R: 2 properties and single RProxy, so these changes reflect the actual state.
We can use R name for the common class (instead of RProxy), naming does not really matter, but we can't use Ti.Android.Rbecause it is also type ofTi.App.Android.R(anyway this common class/interface will not be exported from the module and only used internally as type for theR` properties).

@janvennemann
Copy link
Contributor

@drauggres We should be able to have Titanium.Android.R as both a value and a type, TypeScript supports that. Here's what i think needs to be done to solve this:

  • Get rid of the additional R.yml under titanium/app/android (as you already did). Change the type of the R property in titanium/Android.yml to the existing Titanium.Android.R.
  • Keep the R.yml under titanium/android with it's existing name Titanium.Android.R
  • Change the TypeScript generator so it declares Titanium.Android.R as an interface instead of a namespace. Since TypeScript treats namespaces as values and interfaces as types, we can have both the R property on the Titanium.Android namespace, as well as the type definition Titanium.Android.R as an interface.

@drauggres
Copy link
Contributor Author

Keep the R.yml under titanium/android with it's existing name Titanium.Android.R

Then we'll have Ti.Android.R as type of Ti.App.Android.R. Don't you find it a bit confusing?

@janvennemann
Copy link
Contributor

@drauggres No not really. As you said it is solely used internally to provide typing information. There is no way to create this type so users only access it through the properties Ti.Android.R and Ti.App.Android.R. With your updated summaries on the type and the properties, it should make it clear what's what.

Type:

The Titanium binding of the native Android R class, giving access to Android system-wide resources or application resources.

And then the property definitions state:

Titanium.Android.R

Accessor for Android system resources.

Titanium.App.Android.R

The R namespace for application resources

@drauggres
Copy link
Contributor Author

  • R moved back to Titanium.Android.R.
  • Type of Ti.App.Android.R changed to Ti.Android.R.

@janvennemann janvennemann merged commit 62d3f58 into tidev:master Mar 5, 2020
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.

3 participants