Skip to content

Conversation

@drochetti
Copy link
Contributor

Notes:

Major content reorganization of the Amplify.DataStore. Given the user feedback and issues we've got mainly on the iOS and Android repos, I did a full review of the content when migrating to the new site. Some of the main changes involved:

  • Reusing the same content regarding concepts for all three platforms:
    • reduced code duplication (most of the fragments are code snippets)
    • important callouts done per platform
    • same examples and use cases across all platforms
  • Fixed code snippets and verified their correctness
    • most of it. Some of the examples take a really long time to verify across all three platforms, these are definitely in a better state than before but it will require follow-up work
    • call-out missing features on some platforms (mainly on Android)
  • Simplified some of the content and linked to external sites when needed (e.g. Apple's Combine Framework, AppSync's auto-merge)
  • minor FE improvement: made the line numbers color lighter so customers keep the focus on the code (yeah, I know, the FE Engineer in me can't help himself)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@jamesonwilliams jamesonwilliams left a comment

Choose a reason for hiding this comment

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

Wowser, lots of changes! Hey @drochetti, what is the best way to evaluate the new flow, and the new rendered markdown? Is there way to preview the entire set of changes?

@drochetti
Copy link
Contributor Author

Wowser, lots of changes! Hey @drochetti, what is the best way to evaluate the new flow, and the new rendered markdown? Is there way to preview the entire set of changes?

yeah, sorry for the huge PR. The good news is the content is mostly the same, I just removed the duplicated content across all platforms and extracted mostly the code snippets and platforms callout into platform-specific fragments. I fixed the faulty ones, but the flow of the documentation is mostly the same (if anything I simplified a bit, hence the number of removed lines).

Regarding the preview, not sure why the Amplify Web Preview is not enabled here, let me check that.

@renebrandel
Copy link
Contributor

@drochetti - the previews are only enabled on PRs from branches of aws-amplify/docs repo. If you forked the repo into your own repo, the previews aren't enabled :-(

@undefobj
Copy link
Contributor

undefobj commented Apr 7, 2020

That's too bad, we used forked review in the past by self hosting on GitHub

@undefobj
Copy link
Contributor

undefobj commented Apr 7, 2020

@drochetti After looking at this, it is indeed difficult to review without having the site rendered. My recommendation is one of the following:

  1. We hop on a 10-15 min call tomorrow and look at the rendered content on your system if it's running via screen share
  2. We push your changes, review async, do another round of updates

IMO # 1 is the best as it's less back and forth and iterative reviews. We can make @dabit3 optional on the call. Thoughts?

@drochetti
Copy link
Contributor Author

@drochetti After looking at this, it is indeed difficult to review without having the site rendered. My recommendation is one of the following:

  1. We hop on a 10-15 min call tomorrow and look at the rendered content on your system if it's running via screen share
  2. We push your changes, review async, do another round of updates

IMO # 1 is the best as it's less back and forth and iterative reviews. We can make @dabit3 optional on the call. Thoughts?

Yeah, my bad. I was used to make changes using the forked review and didn't realize the web preview didn't work until it was too late =/

We can definitely do # 1.

@renebrandel renebrandel merged commit b03f9a1 into aws-amplify:v2-preview Apr 7, 2020
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