-
Notifications
You must be signed in to change notification settings - Fork 42
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
docs(clients): add onboarding for new influxdb3-go
, influxdb3-csharp
, influxdb3-java
#6690
docs(clients): add onboarding for new influxdb3-go
, influxdb3-csharp
, influxdb3-java
#6690
Conversation
influxdb3-go
, influxdb3-csharp
influxdb3-go
, influxdb3-csharp
influxdb3-go
, influxdb3-csharp
, influxdb3-java
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.
Overall, I'm glad these libraries are a thing now. However, this PR needs some changes.
- These new client libraries are for Serverless, not TSM. The home pages and guides differ (TSM has Flux guides, and Serverless has SQL guides). The TSM home page should not be modified in this PR.
- The "Application Code" sections of "Add Data" and "Query Data" need updating on the home page to have the appropriate language options open the new guides.
The Add Data section dropdown should still contain all the languages, however, when Go, C#, or Java are selected the new guides should be presented.
The Query Data section dropdown should only show C#, Go, Java, and Python (the ones with SQL guides).
-
The Client Libraries section of the Load Data page should have the C# and Java cards updated to include the "write | query" tag. These cards should also link to the new guides.
-
We're trying not to use the term IOx going forward so more appropriate terminology should be used. "Serverless" when talking about the service, and other appropriate terms such as "InfluxDBClient", "Write" or "Query" when naming things in the example code.
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 is the wrong file. The file that should be edited is HomepageContents.tsx
since this is for Serverless accounts, not TSM accounts.
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.
Fixed.
results of what we have written. | ||
</p> | ||
<p> | ||
Add the following code to the <code>IOxExample</code> class: |
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.
Can we call this class something more specific to what they're doing than "IOxExample"?
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.
Renamed to WriteQueryExample
.
|
||
namespace InfluxDB3.Examples.IOx; | ||
|
||
public class IOxExample |
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.
Rename this class to be something more meaningful to the end-user.
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.
Renamed to WriteQueryExample
.
using InfluxDB3.Client; | ||
using InfluxDB3.Client.Write; | ||
|
||
namespace InfluxDB3.Examples.IOx; |
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.
Avoid the term IOx if possible.
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.
Renamed to InfluxDB3.Examples
onCopy={this.logCopyCodeSnippet} | ||
text="dotnet add package InfluxDB3.Client" | ||
/> | ||
</> |
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.
Are there any other things that the user needs to have installed to make this work properly? Are there version requirements that must be met to avoid errors? If so, please add them here. See the Go and Python guides for examples of this.
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.
No. The dotnet add package InfluxDB3.Client
install latest stable version of the client with all required dependencies.
onCopy={this.logCopyCodeSnippet} | ||
text={gradleDependency} | ||
/> | ||
</> |
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.
Add links to other dependencies or specify minimum versions of things needed to get this running (including Maven & Gradle). (See Python and Go examples)
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.
Fixed
import {getBuckets} from 'src/buckets/actions/thunks' | ||
import {event} from 'src/cloud/utils/reporting' | ||
|
||
const logCopyCodeSnippet = () => { |
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.
Scope these functions within the component.
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.
Fixed
/> | ||
) | ||
} | ||
case 8: { |
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.
There is no case 8, this is redundant.
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.
Fixed
/> | ||
) | ||
} | ||
case 8: { |
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.
Redundant, there is no case 8.
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.
Fixed
Co-authored-by: Daniel Campbell <[email protected]>
Co-authored-by: Daniel Campbell <[email protected]>
@mavarius Thanks for your review, I've fulfilled all requirements... the |
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.
Looks good. The only thing left is to ensure the dropdown doesn't get cut off and scrolls instead. I think after that we'll be good to go. Thanks!
@@ -46,6 +46,8 @@ export const QueryDataAccordion: FC = () => { | |||
const languageList = { |
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.
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.
The list is sorted alphabetically.
I'm unable to modify the scss to display the entire list. For the Add Data dropdown, it works because there's sufficient space beneath it:
A quick but inelegant solution would be to set the max-height to 75px. However, this seems like an unsightly workaround to me.
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.
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.
Looks good, I think it's ready for merge once the tests are passed.
@@ -80,7 +80,7 @@ export const QueryDataAccordion: FC = () => { | |||
/> | |||
<OptionAccordionElement | |||
elementTitle="Application Code" | |||
elementDescription="Write data into your database directly with your application code." | |||
elementDescription="Query your data directly with your application code." |
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.
Good catch
@mavarius, the I've created a new PR with the same content within this repository and all checks have passed: #6753. Could you please approve #6753? Thank you for your reviews. 👍 |
golang
wizard to use new https://github.com/bonitoo-io/influxdb3-go forioxOnboarding
java
wizard + use new https://github.com/bonitoo-io/influxdb3-java forioxOnboarding
c#
wizard + use new https://github.com/bonitoo-io/influxdb3-csharp forioxOnboarding
GO
CSharp
Java
Checklist
Authors and Reviewer(s), please verify the following: