Skip to content
This repository has been archived by the owner on Sep 22, 2023. It is now read-only.

DDI-317 Adding Client-side vs Server-side doc #486

Merged
merged 6 commits into from Jan 9, 2023

Conversation

caseyamp
Copy link
Contributor

@caseyamp caseyamp commented Jan 5, 2023

Amplitude Developer Docs PR

Adds client vs server side explainer doc and reorganizes SDK catalogs for client side versus server side.

Deadline

Whenever

Change type

  • Doc update.
  • New documentation.

PR checklist:

  • My documentation follows the style guidelines of this project.
  • I previewed my documentation on a local server using mkdocs serve.
  • Running mkdocs serve didn't generate any failures.
  • I have performed a self-review of my own documentation.

@caseyamp caseyamp added documentation Improvements or additions to documentation new doc New documentation! labels Jan 5, 2023
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ MARKDOWN markdownlint 5 0 0.25s
✅ MARKDOWN markdown-link-check 5 0 4.94s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@aws-amplify-us-east-1
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-486.d19s7xzcva2mw3.amplifyapp.com

@@ -7,7 +7,14 @@ Use Amplitude SDKs to send event data from your apps into Amplitude.

## Data SDKs

--8<-- "includes/data-sources-sdks.md"
### Data client-side SDKs
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "Data" and "SDKs" parts of these headers seem superfluous, since the parent header is "Data SDKs"

E.g.

## Data SDKs

### Client-side

### Server-side

Same goes for experiment too, for that matter

Copy link
Contributor Author

@caseyamp caseyamp Jan 5, 2023

Choose a reason for hiding this comment

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

@bgiori I did it this way for clarity in slugs/linking -- I'd rather be a little verbose in the heading and get a permalink with #data-client-side-sdks instead of client-side-1 or client-side on a page with multiple duplicate headings.

If you think these headings are hella distracting and the benefit doesn't outweigh the distraction, I'm happy to revisit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO visual clarity is more important. Anecdotally I dont think I've ever actually looked at a header link in a url. Are you more focussed on SEO?

Also, just noticed. Why is this labeled as "Data" SDKs rather than "Analytics" SDKs? Everywhere else in the code, and in the docs we call them "Analytics SDKs"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bgiori I'm not necessarily more focused on SEO, but it is part of my job. More than half of our referrers are from Google Search so I need to spend some effort optimizing things for max findability. Specifically, slugs are used in search ranking and general SEO stuff and are important for a couple other reasons that matter to me:

  1. Page anchor reporting in Amplitude is more clear with explicit header/anchor links -- When I'm looking at charts, I don't want to have to navigate to a page anchor in the docs to know what's there. This matters specifically here because our chart granularity is to the page, so having several similar anchor links in the same doc is creates a problem.
  2. It's also easier for maintainers to figure out what's pointing where when it has an explicit title like analytics-client-side-sdks. People sometimes modify headers without thinking that things might be linked to it, and having explicit header links make it easier to figure out where the broken link is supposed to go. This happens frequently enough that I care about it.

Maintainer/contributor experience is also a factor we need to include when making these sorts of decisions.

All that being said, this isn't a hill I care about dying on, so if you feel strongly that the headings are no good, I can change them with no hard feelings.

As for the "Data" thing -- I don't know why wrote it that way, good catch. I'll change it to Analytics.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't want to die on a hill either, at least not now, I've still got plenty of life left in me. I'll defer 🥲

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll try it my way and if it turns out terrible, we'll change it. None of this has to be permanent.

- :java: [Java](../../data/sdks/java/)
- :node: [Node.js](../../data/sdks/typescript-node/)
- :python: [Python](../../data/sdks/python/)
- :react: [React Native](../../data/sdks/typescript-react-native/)
Copy link
Collaborator

Choose a reason for hiding this comment

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

React Native is a client-side SDK, should be removed from this section of the list

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 knew there was at least one dumb copy-paste error left somewhere. Thanks!

@caseyamp caseyamp merged commit 611191e into main Jan 9, 2023
@caseyamp caseyamp deleted the DDI-317.ClientSidevsServer branch January 9, 2023 19:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation new doc New documentation! roadmap work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants