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

[media-library] add doc comments in source, correct types #13936

Merged
merged 3 commits into from
Aug 11, 2021

Conversation

Simek
Copy link
Collaborator

@Simek Simek commented Aug 5, 2021

Why

Fixes ENG-1458

To utilize the auto generation feature for API section on the docs website the package source code must contain the relevant documentation.

How

This PR moves the current documentation of MediaLibrary API - https://docs.expo.io/versions/latest/sdk/media-library - with minor tweaks and additions to the package source code.

This was one of the most demanding conversion so far mainly by the shear amount of content and types variation, however not 100% complete, there are still small parts and descriptions missing, it should be way more complete than the current version of the page.

To full support the migration to automated docs extraction I had to introduce few improvements and fixes to the code:

  • add missing AlbumType and MediaSubtype
  • add missing orientation key to the Asset type
  • add missing return types for few methods
  • simplify PermissionResponse and MediaLibraryAssetsChangeEvent types
  • export missing types: EXPermissionResponse, Subscription

I have also added small fix for the Event Subscription section, which should now include also method using plural of listerner.

However there is still a room for improvement, especially I have in mind MediaType and SortBy constants, which definitely should be redefined to way simpler construction, so I did not touch their "internal" types like MediaTypeValue, MediaTypeObject or SortByValue.

Also I'm not sure why removeSubscription method exist, it was not earlier listed in the docs.

Test Plan

The changes and generated content have been tests running docs website on localhost.

Preview

Screenshot 2021-08-05 at 17 59 46

Checklist

  • Documentation is up to date to reflect these changes (eg: https://docs.expo.io and README.md).
  • This diff will work correctly for expo build (eg: updated @expo/xdl).
  • This diff will work correctly for expo prebuild & EAS Build (eg: updated a module plugin).

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Aug 5, 2021
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Aug 5, 2021
@expo-bot
Copy link
Collaborator

expo-bot commented Aug 6, 2021

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing changelog entries


Your changes should be noted in the changelog. Read Updating Changelogs guide and consider (it's optional) adding an appropriate entry to the following changelogs:


Generated by ExpoBot 🤖 against 45e1f17

@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: passed checks ExpoBot has nothing to complain about labels Aug 6, 2021
Copy link
Contributor

@bbarthec bbarthec left a comment

Choose a reason for hiding this comment

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

Great job 👍 Many thanks for your hard work on this demanding module 👏

@bbarthec bbarthec merged commit 04eb122 into expo:master Aug 11, 2021
@Simek Simek deleted the media-library-doc-comments-to-source branch August 11, 2021 17:39
FelipeACP pushed a commit to FelipeACP/expo that referenced this pull request Sep 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: suggestions ExpoBot has some suggestions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants