-
Notifications
You must be signed in to change notification settings - Fork 80
Update netsuite-suiteanalytics.md #2512
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
base: master
Are you sure you want to change the base?
Conversation
|
jwhartley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Hah, you've left the LLM writing at the top of the change - please remove that
- Actual docs change looks good
- There's a dash added at the bottom of the file, do you know what that's for?
- The name of the doc in the preview is showing as 'netsuite-suiteanalytics' in the sidebar, I assume this is because of the LLM writing at the top taking away the title inference, but please double-check when updated
Changed a few key details per James' recommendations and notes.
jwhartley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved now
aeluce
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR will need some changes before merging: it causes some breakages, introduced in the latest update (note that the PR preview deployment is now failing its check). For any changes you're unsure of, you can also run the docs site locally to test things out.
| @@ -1,5 +1,3 @@ | |||
| import ReactPlayer from "react-player"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should not be removed: we need to import ReactPlayer since we embed a video on this page.
| log_cursor: lastmodifieddate | ||
| target: ${PREFIX}/${CAPTURE_NAME}/transaction | ||
| {...} | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line of triple backticks is needed to close off the code block.
Description:
(Describe the high level scope of new or changed features)
Workflow steps:
(How does one use this feature, and how has it changed)
Documentation links affected:
(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)
Notes for reviewers:
(anything that might help someone review this PR)