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

[No QA] Create a readme for API details #9488

Merged
merged 14 commits into from
Jun 21, 2022
Merged

[No QA] Create a readme for API details #9488

merged 14 commits into from
Jun 21, 2022

Conversation

tgolen
Copy link
Contributor

@tgolen tgolen commented Jun 17, 2022

Details

This contains most of the answers I've been giving to questions from people.

Fixed Issues

None

Tests

None

QA Steps

None

@tgolen tgolen requested a review from a team as a code owner June 17, 2022 20:44
@tgolen tgolen self-assigned this Jun 17, 2022
@tgolen tgolen changed the title Create a readme for API details [No QA] Create a readme for API details Jun 17, 2022
@melvin-bot melvin-bot bot requested review from amyevans and removed request for a team June 17, 2022 20:44
@tgolen
Copy link
Contributor Author

tgolen commented Jun 21, 2022

bump @amyevans for review

amyevans
amyevans previously approved these changes Jun 21, 2022
Copy link
Contributor

@amyevans amyevans left a comment

Choose a reason for hiding this comment

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

Looks good, and reading this thoroughly helped cement a few things for me, so definitely appreciating the addition! I've left a few non-blocking comments, feel free to merge instead if you'd like!

README.md Outdated
- All data that is brought into the app and is necessary to display the app when offline should be stored on disk in persistent storage (eg. localStorage on browser platforms). [AsyncStorage](https://reactnative.dev/docs/asyncstorage) is a cross-platform abstraction layer that is used to access persistent storage.
- All data that is displayed, comes from persistent storage.
1. **UI Binds to data on disk**
4. **UI Binds to data on disk**
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB, but with these updates, 3 is now missing. I've seen (and like) the convention of numbering them all 1 (since they don't matter anyway -- an <li> tag just gets generated regardless of the number). Let's either return them to 1 or renumber to include 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, sure. PHPStorm likes to automatically renumber them, and I agree with using all 1s. I'll update that.

docs/API.md Outdated
@@ -0,0 +1,53 @@
# API Details
These are best practices related to the current API used for App. This does not relate to any of our `DeprecatedAPI`.
# Philosophy
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB, but since you've already got an h1 above, I'd recommend bumping all subsequent headings in the doc down a level (e.g. this to h2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea

docs/API.md Outdated

Pusher listens for an `onyxApiUpdate` event and sends the data straight to `Onyx.update(pushJSON)`.
## READ Responses
This is a response that returns data from the database that didn't exist before.
Copy link
Contributor

Choose a reason for hiding this comment

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

"didn't exist before" - this phrase is slightly confusing to me. I believe it means data that wasn't previously stored in Onyx on the client, but could you reword it to be crystal clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I completely mis-worded this. Will fix

Co-authored-by: Amy Evans <amy@expensify.com>
tgolen and others added 5 commits June 21, 2022 12:48
Co-authored-by: Amy Evans <amy@expensify.com>
Co-authored-by: Amy Evans <amy@expensify.com>
@tgolen
Copy link
Contributor Author

tgolen commented Jun 21, 2022

Updated

@amyevans amyevans merged commit 88a2bdd into main Jun 21, 2022
@amyevans amyevans deleted the tgolen-update-readme branch June 21, 2022 19:22
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @amyevans in version: 1.1.78-4 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @sketchydroide in version: 1.1.78-8 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

3 participants