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

Document Java Source Context #7086

Merged
merged 32 commits into from
Jun 7, 2023
Merged

Document Java Source Context #7086

merged 32 commits into from
Jun 7, 2023

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Jun 5, 2023

Pre-merge checklist

If you work at Sentry, you're able to merge your own PR without review, but please don't unless there's a good reason.

  • Checked Vercel preview for correctness, including links
  • PR was reviewed and approved by any necessary SMEs
  • PR was reviewed and approved by a member of the Sentry docs team

Description of changes

Describe your changes here. If your PR relates to or resolves an issue, add a link to that too.
See getsentry/sentry-java#633 for more details.

@adinauer adinauer requested review from kamilogorek and a team as code owners June 5, 2023 10:56
@vercel
Copy link

vercel bot commented Jun 5, 2023

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

Name Status Preview Comments Updated (UTC)
sentry-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 7, 2023 9:15am

@adinauer
Copy link
Member Author

adinauer commented Jun 5, 2023

@smeubank @stephanie-anderson preview is now working :-)

Copy link
Member

Choose a reason for hiding this comment

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

i'd like to proceed with approving and merging these changes, but I'd like us to consider updating these pages to work like the JS source maps pages today

  • meaning a little less text
  • similar image to show before and after
  • set up sub pages instead of lists of each option for plugin config

Ideally this should make the page easier to consume and things like UUIDs for debug files should be hidden away

I will take a crack at it later if I get time

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be awesome if we can split up the source code context in a product's page (explaining the feature but platform agnostic) so that platforms can just link to when they offer support.

This is tracked here https://www.notion.so/sentry/x-Source-Context-adoption-83a34f599c9745498aa20849ac44fdc8?pvs=4#43bdef3f5b2142e59b180fafae6e3bf8

Then the source code context could add includes (with code snippets) on how to install per platform, this makes the content/feature more discoverable and easy to add support.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that restructuring would help these docs a lot! I really like the idea of splitting the source code context into a product page.

Copy link
Member

@smeubank smeubank left a comment

Choose a reason for hiding this comment

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

LGTM I just want to follow up with a restructuring PR based on how JS is setup, I think it might be a little easier to consume for users looking at different platforms if they are the same structure, and it focuses that users don't need to scroll around content they are not concerned with

And yea some minor points about remocing code comments in snippets

Other wise let's gooooo! :D

Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if this should live in one place in java docs and from product docs we could link there, or it could be an include that we add instead

again I think this could be in a restructuring PR to not delay this

src/wizard/android/index.md Outdated Show resolved Hide resolved
src/wizard/android/index.md Outdated Show resolved Hide resolved
src/wizard/java/index.md Outdated Show resolved Hide resolved
src/wizard/java/logback.md Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

yea this is something we need to cleanup when work on improving java onboarding in wizard

Copy link
Member

Choose a reason for hiding this comment

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

image

shouldn't the v3 snippets appear first by default? to get most people on latest versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we did this order back when v3 and our support for it was very new. Can change in a follow up PR.

Copy link
Member

Choose a reason for hiding this comment

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

same things, latest first? would be nice if the who page could toggle between maven and gradle but that is too complex something to handle in onboarding wizard though for sure

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this tab thin doesn't work in the wizard. We can figure out maven/gradle and other toggles in getsentry/sentry#50226

Copy link
Member

@markushi markushi 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 to me! I've created a small PR which also integrates source context into our Android Gradle overview page: #7100

src/wizard/android/index.md Outdated Show resolved Hide resolved
Co-authored-by: Markus Hintersteiner <markus.hintersteiner@sentry.io>
@adinauer
Copy link
Member Author

adinauer commented Jun 6, 2023

@shanamatthews could you please give this a review?

Copy link
Contributor

@shanamatthews shanamatthews left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on this review! I went down a bit of a rabbit hole about how this could be restructured to be easier to follow, but I agree with Steven.

Let's publish this version and follow up with a PR to restructure.

I tried to keep my comments to style guide/grammar fixes and left the structure alone for now.

@smeubank and @adinauer - let me know if you want to have a quick meeting to discuss restructuring this!

src/docs/product/cli/dif.mdx Outdated Show resolved Hide resolved
src/docs/product/cli/dif.mdx Outdated Show resolved Hide resolved
src/docs/product/cli/dif.mdx Outdated Show resolved Hide resolved
src/platform-includes/source-context/java.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that restructuring would help these docs a lot! I really like the idea of splitting the source code context into a product page.

src/wizard/java/spring.md Outdated Show resolved Hide resolved
src/wizard/java/spring.md Outdated Show resolved Hide resolved
src/docs/product/cli/dif.mdx Outdated Show resolved Hide resolved
src/docs/product/cli/dif.mdx Outdated Show resolved Hide resolved
src/docs/product/cli/dif.mdx Outdated Show resolved Hide resolved
adinauer and others added 4 commits June 7, 2023 06:42
Co-authored-by: Shana Matthews <shana.l.matthews@gmail.com>
Co-authored-by: Shana Matthews <shana.l.matthews@gmail.com>
adinauer and others added 4 commits June 7, 2023 07:33
Co-authored-by: Shana Matthews <shana.l.matthews@gmail.com>
Co-authored-by: Shana Matthews <shana.l.matthews@gmail.com>
@adinauer
Copy link
Member Author

adinauer commented Jun 7, 2023

Thanks everyone. Let's do follow up PR(s) for restructuring. Will merge this once https://github.com/getsentry/sentry-android-gradle-plugin 3.9.0 is out as it contains bug fixes required for following the docs here.

@adinauer adinauer merged commit 55c4d20 into master Jun 7, 2023
7 checks passed
@adinauer adinauer deleted the feat/java-source-context branch June 7, 2023 09:29
@github-actions github-actions bot locked and limited conversation to collaborators Jun 23, 2023
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.

None yet

6 participants