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

feat: Add Svelte SDK docs #5413

Merged
merged 7 commits into from
Aug 17, 2022
Merged

feat: Add Svelte SDK docs #5413

merged 7 commits into from
Aug 17, 2022

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Aug 12, 2022

This PR adds official documentation to the new @sentry/svelte JS SDK which is currently in aplha. It includes

  • A disclaimer that this SDK is in alpha and subject to breaking changes
  • Instructions for installing and using the SDK in Svelte projects
  • Instruction for generating and uploading Sourcemaps in Svelte projects

ref: getsentry/sentry-javascript#5573

@Lms24 Lms24 self-assigned this Aug 12, 2022
@vercel
Copy link

vercel bot commented Aug 12, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
sentry-docs ✅ Ready (Inspect) Visit Preview Aug 17, 2022 at 10:03AM (UTC)

src/includes/getting-started-config/javascript.svelte.mdx Outdated Show resolved Hide resolved
@@ -0,0 +1,7 @@
title: Svelte
Copy link
Member

Choose a reason for hiding this comment

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

(Don't know where else to put this.) Do we already have plans to upload a logo for svelte in the docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Haven't done it yet but we either do it when we first publish the docs or when we go into beta. Still need to take a look at how we did it with Remix.

Copy link
Contributor

@imatwawana imatwawana left a comment

Choose a reason for hiding this comment

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

This is mostly ready to go, but I've made some styling edits and a few other suggestions. I'd like to give this another review before you merge. Please tag me when it's ready for me to look at again.

@@ -0,0 +1,12 @@
Add a button to a Svelte component that throws an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

This section already has a lead-in sentence for the code snippet in the common content (see screenshot below):

2022-08-15_14-10-43

So the lead-in text you've added doesn't make sense in this context. I can't make the suggestion directly here because the back ticks from the code snippet interfere, but I'd suggest that you remove the sentence about the button before the code snippet and add the following after the code snippet:

This snippet adds a button that throws an error to a Svelte component.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. I think we have the same problem in the Remix SDK docs (which I took as an example). Will open another PR to adjust this (among some other things I came across)

src/includes/getting-started-config/javascript.svelte.mdx Outdated Show resolved Hide resolved
src/includes/getting-started-primer/javascript.svelte.mdx Outdated Show resolved Hide resolved
src/includes/sourcemaps/generate/javascript.svelte.mdx Outdated Show resolved Hide resolved
src/includes/sourcemaps/generate/javascript.svelte.mdx Outdated Show resolved Hide resolved
src/includes/sourcemaps/generate/javascript.svelte.mdx Outdated Show resolved Hide resolved
src/includes/sourcemaps/overview/javascript.svelte.mdx Outdated Show resolved Hide resolved
@Lms24
Copy link
Member Author

Lms24 commented Aug 16, 2022

Thanks for the great review @imatwawana

Please tag me when it's ready for me to look at again

Updated the PR with all your suggestions. I think this is ready for another round!

Copy link
Contributor

@imatwawana imatwawana left a comment

Choose a reason for hiding this comment

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

I made one small comment, but otherwise, this looks great!

@Lms24 Lms24 force-pushed the lms-svelte-sdk-docs branch from 2ac24b3 to a055b13 Compare August 17, 2022 09:47
@Lms24 Lms24 merged commit 9098555 into master Aug 17, 2022
@Lms24 Lms24 deleted the lms-svelte-sdk-docs branch August 17, 2022 12:02
@github-actions github-actions bot locked and limited conversation to collaborators Sep 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants