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

Use aws-amplify/docs as foundation #75

Merged
merged 21 commits into from
Jul 12, 2021
Merged

Use aws-amplify/docs as foundation #75

merged 21 commits into from
Jul 12, 2021

Conversation

ericclemmons
Copy link
Contributor

@ericclemmons ericclemmons commented Jul 9, 2021

See: https://app.asana.com/0/1200334286823427/1200549772234080/f

This is pretty tricky, but I'm attempting to re-use https://github.com/aws-amplify/docs/tree/next for its layout & theme around our content.

  • https://localhost:5000/ui is the new root, so that URLs are consistent with docs.amplify.aws/ui
    • This means the location of features & example pages have moved so that URLs are our contract from docs to features to examples to tests.
  • Content still goes in src/content/**/index.mdx, but co-locating now works. Use .page.tsx if you need a non-MDX page.
  • Using xdm instead of @next-js/mdx or next-mdx-* alternatives – they were having issues with getStaticProps and imports.
  • <Feature name="sign-in"> renders a feature table explicitly within the docs.
  • There are still more tweaks to be made, but this course-corrects the layout & architecture to avoid the DX issues we've had while aligning with the end-result for launch.

https://pr-75.d3inl0muob87le.amplifyapp.com/ui

Screen Shot 2021-07-11 at 10 40 07 PM

Screen Shot 2021-07-11 at 10 43 47 PM

Screen Shot 2021-07-11 at 10 44 16 PM


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

Comment on lines +9 to +10
- const st = opts.fs.statSync(pi)
+ const st = opts.fs.lstatSync(pi)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran into an issue with amplify-docs, used patch-package, but found it had an issue with symlinks:

manidlou/node-klaw-sync#18

@ericclemmons ericclemmons self-assigned this Jul 12, 2021
@@ -3,7 +3,7 @@
"name": "docs",
"version": "0.0.1",
"scripts": {
"dev": "PORT=5000 next-remote-watch 'src/content' '../packages/e2e/**/*.feature'",
"dev": "next dev -p 5000",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No more next-remote-watch because no more next-mdx-remote

"@xstate/inspect": "^0.4.1",
"amplify-docs": "github:https://github.com/aws-amplify/docs#33d383d",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Including https://github.com/aws-amplify/docs as a dependency is buggy with yarn.

The best way I've found was to not use #next, but specify a SHA.

Let me know if you can install & run this locally, or if you run into issues.

I had to do yarn cache clean between version upgrades =/


export function XStateInspector() {
return <iframe data-xstate style={{ width: "100%", height: "40ch" }} />;
React.useLayoutEffect(() => {
inspect();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems safer/idiomatic than typeof window checks.

Comment on lines 5 to 6
import { Authenticator } from "@aws-amplify/ui-react";
import { XStateInspector } from "@/components/XStateInspector";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@reesscot Explicit imports work now!

@@ -2,6 +2,8 @@
title: View
---

import { View, ViewDemo } from "@aws-amplify/ui-react"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@reesscot The styles don't seem to be built for this ViewDemo, but that's probably because it's in a separate package still...

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll cut a PR to move all the demos back to docs

Comment on lines 9 to 13
const Content = dynamic(() => import(`../content/${slug}/index.mdx`), {
loading() {
return <div dangerouslySetInnerHTML={{ __html }} />;
},
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is cool little "progressive enhancement" approach:

  • dynamically import the component, but statically render the markup.
  • This way the page renders first & correct with SSG/SSR, but becomes functional onload.

Comment on lines 36 to 37
const { default: Content, frontmatter = {} } = await import(
`../content/${slug}/index.mdx`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can potentially impact build times with webpack (because webpack will code-split all matching paths), but since we have a limited number of pages it doesn't seem much different than builds today.

And, honestly, this is working better than fs.readFileSync, next-mdx-remote, and other solutions.

Comment on lines +16 to +18
<script src="https://cdn.jsdelivr.net/npm/docsearch.js@2.6.3/dist/cdn/docsearch.min.js"></script>
<script src="https://a0.awsstatic.com/s_code/js/3.0/awshome_s_code.js"></script>
<script src="/scripts/shortbreadv1.js"></script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

amplify-docs will throw runtime errors without these scripts 🤷

}

pre {
@apply rounded;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cut myself on those corners!

@ericclemmons ericclemmons marked this pull request as ready for review July 12, 2021 06:17
@ericclemmons ericclemmons requested a review from a team July 12, 2021 06:22
@ericclemmons
Copy link
Contributor Author

@reesscot I just merged in your changes and I think there's an opportunity to do more colocating of files with demos, like your images:

https://github.com/remcohaszing/remark-mdx-images

Copy link
Contributor

@ErikCH ErikCH left a comment

Choose a reason for hiding this comment

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

This looks good to me other then the conflict and the features not working yet. But I'm guessing the features will be added back to the Authenticator page on a future release?

@@ -0,0 +1,113 @@
import {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is replacing FeatureTests.tsx

Comment on lines +37 to +41
## Features

### Sign Up

<Feature name="sign-up" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explicitly putting features into the docs now.

This way we have more control of where they appear & how we describe them.

Feature is responsible for showing the Examples table.

@swaminator
Copy link
Contributor

swaminator commented Jul 12, 2021

Thanks for the work! Know it's still WIP, but couple of questions:

  1. How will we handle platform/framework filtering that exists in today's docs?
  2. If a customer picks 'React' they are able to stay within 'React' throughout the docs site. Will this functionality still work?
  3. How will docs 'search' functionality work with this new site?
  4. When customers hit the 'Edit on GitHub' button in the docs where will they go?
  5. What happens if/when the docs engineering team build out new features? (E.g. a 'Rate this content' feature)
  6. I notice some of the UI components on the page are unique to our content only (e.g. the expander caret here). Is this just a point-in-time thing?

@ErikCH ErikCH merged commit 64bc331 into main Jul 12, 2021
@ErikCH ErikCH deleted the amplify-docs branch July 12, 2021 22:41
@ericclemmons
Copy link
Contributor Author

ericclemmons commented Jul 19, 2021

  1. The framework chooser is coming. We've just started creating per-platform documentation which will use this (Angular POC clean up #89?)

  2. Mostly – we'll know the framework because the primary docs contain the framework in the url: /q/framework/react. The framework wouldn't be persisted if you started with React & switched to Angular on the UI docs, then went back to the library – that's where putting ?framework=react in the URLs addresses this (vs. relying on React Context).

  3. Search is powered by Algolia's crawler, so it'll pick up the new content during their crawl. I'd honestly prefer a separate search for UI, but that doesn't make sense unless we were a separate "product" page like https://ui.supabase.io/.

  4. If we keep the (terrible) "Edit on GitHub" button have today, it'll go to the page source:

    Instead, I want to put the Edit beside the content itself, not the page.

  5. We update the dependency & it's either (1) already part of the layout or (2) we add the component:

    https://github.com/aws-amplify/amplify-ui/tree/main/docs#amplify-ui-documentation

    I've spoken to @jakeburden about an engineering task to make the docs a Next.js plugin that's source-agnostic – right now, the components are tightly coupled to the docs' filesystem.

  6. Yes, we'll have UI-specific docs & features to accompany them. What I think you're referring to is the HTML <details> element. (Interactive demos, links to tests & features, etc. are other UI-specific features)

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.

None yet

4 participants