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

Doc requirements for platform api expectations #1

Merged
merged 4 commits into from
Sep 6, 2018

Conversation

audreyso
Copy link
Contributor

@audreyso audreyso commented May 11, 2017

Ready for review!

Platforms affected

What does this PR do?

Document requirements for platform api expectations

What testing has been done on this change?

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

PlatformRequirements.md Outdated Show resolved Hide resolved
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Woo, awesome job. This is a fantastic start. I dropped like a million comments and I think there's a bit more work to do but we're definitely on the right track.

Other than my specific comments, a few overall nits:

  • can we line up the reference API documentation so it is a bit more similar to, for example, the plugin API documentation? e.g. the Media plugin ref docs?
  • can we ensure to backtick any code object references or plugin names?
  • I would be down to move this into the README than keep it in a separate document. My two cents.

Thanks for taking this on @audreyso ! I think nailing this document will really reduce the barrier to entry to contributing to Cordova. Keep up the good work.

PlatformRequirements.md Show resolved Hide resolved
- bin/create.bat for windows
- windows .bat file typically just calls bin/create with node

bin/update
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove this, as the update script seems to be going away, e.g. https://issues.apache.org/jira/browse/CB-12948

PlatformRequirements.md Outdated Show resolved Hide resolved
PlatformRequirements.md Outdated Show resolved Hide resolved
PlatformRequirements.md Outdated Show resolved Hide resolved
PlatformRequirements.md Outdated Show resolved Hide resolved
PlatformRequirements.md Outdated Show resolved Hide resolved
PlatformRequirements.md Outdated Show resolved Hide resolved
PlatformRequirements.md Outdated Show resolved Hide resolved
- path to local repo of valid plugin: /my/cordova/repositories/cordova-plugin-globalization
- The `removePlugin` method must return a promise either fulfilled or rejected with a CordovaError instance.

- CLI work flow integration
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it is worth linking to / expanding on the two different kinds of workflows cordova supports? I.e. CLI-based workflow (supporting multiple platforms) or single-platform-based workflow that purely leverages the platform API. e.g. the top of the Android guide describes there are "platform centered shell tools" or "cross-platform Cordova CLI" workflows.

@purplecabbage
Copy link
Contributor

Another thing to note is that the tests that are in this repo could be used to verify the platformAPI for anyone attempting to implement it.

@audreyso audreyso force-pushed the DocRequirements branch 13 times, most recently from ea28b27 to 839c321 Compare July 25, 2017 23:16
Copy link
Contributor Author

@audreyso audreyso left a comment

Choose a reason for hiding this comment

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

After reading through the comments, I made a bunch of updates! Please let me know:

  • if there's any comment that I misunderstood or missed
  • if any links are not correct
  • how the format looks and if anything needs to be changed there

Thanks so much! :)

PlatformRequirements.md Outdated Show resolved Hide resolved
PlatformRequirements.md Outdated Show resolved Hide resolved
@janpio
Copy link
Member

janpio commented Sep 3, 2018

This seems almost ready to merge.

What about those last two comments @filmaj @purplecabbage @audreyso? Do you have suggestions what the text should be replaced with so this can be taken care of and merged?

Copy link
Contributor

@purplecabbage purplecabbage left a comment

Choose a reason for hiding this comment

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

Lgtm!

@janpio
Copy link
Member

janpio commented May 3, 2019

Went through the review comments and resolved the ones that were definitely taken care of before merging, left the ones that are not 100% explained or done yet and might need a new PR to fix.

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.

5 participants