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.

Sign up for GitHub

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

Jump to bottom

RUMM-1769 Add Objective-C documentation snippets #705

Merged
merged 5 commits into from
Feb 1, 2022
Merged

RUMM-1769 Add Objective-C documentation snippets #705

merged 5 commits into from
Feb 1, 2022

Conversation

maxep
Copy link
Member

@maxep maxep commented Jan 4, 2022

What and why?

Add Objective-C version to all snippets when applicable.

Review checklist

  • Make sure each commit and the PR mention the Issue number or JIRA reference

@maxep maxep requested review from a team as code owners January 4, 2022 13:29
maxep
maxep commented Jan 4, 2022
View reviewed changes
docs/log_collection.md
{{< tabs >}}
{{% tab "US" %}}

{{< site-region region="us" >}}
Copy link
Member Author

@maxep maxep Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Use the same format as Android. Should we provide all regions?

Copy link
Contributor

@alai97 alai97 Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Bumping this question about region support: would be good to include support/no support for US3, US5, and Gov.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can also add this in another doc card too (thanks for your patience)!

@maxep maxep force-pushed the maxep/RUMM-1769/documentation-snippets branch from 7a595bb to 56c59ae Compare January 4, 2022 13:45
@maxep maxep self-assigned this Jan 4, 2022
ncreated
ncreated reviewed Jan 5, 2022
View reviewed changes
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

The snippet in DDUIKitRUMViewsPredicate still needs translation, but in general this looks great - nice work!

Two questions though:

  • Shouldn't we use nil instead of NULL all around? AFAIK nil is more idiomatic for Objective-C and NULL is/was used to satisfy C/++ code, no?
  • Have we tried previewing this doc with docs website running locally? Would be good to see if all tabs are consistent before we deploy it.

docs/rum_collection/advanced_configuration.md Show resolved Hide resolved
docs/rum_collection/advanced_configuration.md Outdated Show resolved Hide resolved
@maxep
Copy link
Member Author

maxep commented Jan 5, 2022

Shouldn't we use nil instead of NULL all around? AFAIK nil is more idiomatic for Objective-C and NULL is/was used to satisfy C/++ code, no?

True, too long I haven't touched objc :)

Have we tried previewing this doc with docs website running locally? Would be good to see if all tabs are consistent before we deploy it.

Not yet, I wasn't able to do so last time I tried. I will give it a shot.

ncreated
ncreated approved these changes Jan 5, 2022
View reviewed changes
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

👌👌

buranmert
buranmert suggested changes Jan 6, 2022
View reviewed changes
Copy link
Contributor

@buranmert buranmert left a comment

Choose a reason for hiding this comment

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

i don't how these markdowns are rendered in actual docs pages, if they look good then ignore my alignment issue comments

docs/log_collection.md Outdated Show resolved Hide resolved
docs/log_collection.md Outdated Show resolved Hide resolved
docs/rum_collection/_index.md Outdated Show resolved Hide resolved
docs/rum_collection/_index.md Outdated Show resolved Hide resolved
docs/rum_collection/_index.md Outdated Show resolved Hide resolved
docs/rum_collection/advanced_configuration.md Outdated Show resolved Hide resolved
docs/trace_collection.md Outdated Show resolved Hide resolved
docs/trace_collection.md Outdated Show resolved Hide resolved
@maxep maxep force-pushed the maxep/RUMM-1769/documentation-snippets branch from ccecf59 to a0b499b Compare January 10, 2022 15:57
@maxep maxep requested a review from buranmert January 10, 2022 16:12
@maxep maxep force-pushed the maxep/RUMM-1769/documentation-snippets branch from a0b499b to 6f1c83b Compare January 14, 2022 13:07
@alai97
Copy link
Contributor

alai97 commented Jan 27, 2022

Hey @maxep, I'm creating a test documentation branch now so you can see the doc site preview!

@alai97
Copy link
Contributor

alai97 commented Jan 27, 2022

I just opened this test PR (#12928) for you to preview the changes in the doc site! https://docs-staging.datadoghq.com/alai97/RUMM-1769-doc-test-update/logs/log_collection/ios/#pagetitle and https://docs-staging.datadoghq.com/alai97/RUMM-1769-doc-test-update/real_user_monitoring/ios/ for example 🙃

alai97
alai97 approved these changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@alai97 alai97 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 for docs!

@maxep
Copy link
Member Author

maxep commented Jan 28, 2022

Thanks @alai97! But I don't see the changes in https://docs-staging.datadoghq.com/alai97/RUMM-1769-doc-test-update/real_user_monitoring/ 🤔
All code snippets should be in tabs with Swift and Objective-C.

@alai97
Copy link
Contributor

alai97 commented Jan 28, 2022 •
edited
Loading

@maxep are you expecting to see an update on the product landing page you linked to?

I just fixed my doc branch, can you try again?

@alai97 alai97 mentioned this pull request Jan 29, 2022
Closed
4 tasks
@xgouchet
Copy link
Member

xgouchet commented Feb 1, 2022

Seems like @alai97's branch is functionnal with tabs for Swift/Obj C

@maxep
Copy link
Member Author

maxep commented Feb 1, 2022

It's working now, thank you @alai97 🙏

@maxep maxep merged commit 4bbf6a9 into master Feb 1, 2022
@maxep maxep deleted the maxep/RUMM-1769/documentation-snippets branch March 6, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alai97 alai97 alai97 approved these changes

@ncreated ncreated ncreated approved these changes

@buranmert buranmert buranmert approved these changes

@xgouchet xgouchet xgouchet approved these changes

Assignees

@maxep maxep

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maxep @alai97 @xgouchet @buranmert @ncreated