-
Notifications
You must be signed in to change notification settings - Fork 1
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 remaining SDK instructions #164
Conversation
# Installation steps | ||
If you want to skip ahead, the final code is available in our [GitHub repository](https://github.com/launchdarkly/hello-android). |
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.
commented this on the last PR as well but I think we'll probably want to pull this out and add to the top of the View for the show sdk step instead of having them each in here.
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.
Yeah, that makes sense.
@@ -176,13 +177,18 @@ func sendFetchEnv(accessToken string, baseUri string, key string, projKey string | |||
var resp struct { | |||
SDKKey string `json:"apiKey"` | |||
ClientSideId string `json:"_id"` | |||
MobileKey string `json:"mobileKey"` |
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.
pulling out the env's mobile key now as well
|
||
You should see: | ||
|
||
`Feature flag my-flag-key is FALSE` |
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.
I don't know why but this last line isn't showing up for me in my terminal 🤔
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.
I'm able to see it, even after changing the terminal height. 🤷
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.
Actually I ran into a lot of inconsistency bugs when viewing these in the terminal. Lines duplicating. This line not showing. I figure it was a bug with our scrolling container. I think we should open a new ticket for that.
You should see: | ||
`Feature flag my-flag-key is FALSE for this context` |
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.
similar to haskell - these two last lines aren't showing up 🤔
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.
Wonder if I'm failing to escape characters somewhere. I'll investigate
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.
it might just be me if danny's able to see it 🤷♀️
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.
Ok. I did see it too before. I was testing in the vscode terminal not in its own window
You should see: | ||
`Feature flag my-flag-key is FALSE for this context` |
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.
missing lines 🤔
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.
Sorry am I missing something here?
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.
sorry no same thing that might just be me where i can't see these last lines
@@ -108,35 +108,36 @@ func (s sdkDetail) FilterValue() string { return "" } | |||
|
|||
var SDKs = []sdkDetail{ | |||
{canonicalName: "react", displayName: "React", kind: clientSideSDK, hasInstructions: true}, | |||
{canonicalName: "node-server", displayName: "Node.js (server-side)", kind: serverSideSDK}, | |||
{canonicalName: "node-server", displayName: "Node.js (server-side)", kind: serverSideSDK, hasInstructions: true}, |
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.
I'll add a follow up PR to remove this extra check now that we're no longer fetching these from the README's
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 looks good to go after a few small changes.
internal/quickstart/choose_sdk.go
Outdated
{canonicalName: "c-server", displayName: "C/C++ (server-side)", kind: serverSideSDK, hasInstructions: true}, | ||
{canonicalName: "lua-server", displayName: "Lua", kind: serverSideSDK, hasInstructions: true}, | ||
{canonicalName: "haskell-server", displayName: "Haskell", kind: serverSideSDK, hasInstructions: true}, | ||
{canonicalName: "apex-server", displayName: "Apex", kind: serverSideSDK, hasInstructions: true}, |
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.
I think we can remove apex and electron since they're not in the UI SDK list.
# Installation steps | ||
If you want to skip ahead, the final code is available in our [GitHub repository](https://github.com/launchdarkly/hello-android). |
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.
Yeah, that makes sense.
class MainActivity : AppCompatActivity() { | ||
|
||
// Set BOOLEAN_FLAG_KEY to the boolean feature flag you want to evaluate. | ||
val BOOLEAN_FLAG_KEY = "my-flag-key" |
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 like we still have this bug in a couple places. We don't need to fix that in this PR, but could you fix it in a separate one?
|
||
You should see: | ||
|
||
`Feature flag my-flag-key is FALSE` |
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.
I'm able to see it, even after changing the terminal height. 🤷
Co-authored-by: Kelly Hofmann <55991524+k3llymariee@users.noreply.github.com>
.name(getUserName()) | ||
.build() | ||
} else { | ||
LDContext.builder(ContextKind.DEFAULT, "example-user-key") |
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 you change "example-user-key" to "my-flag-key" so it gets interpolated? This is the code that lists expected keys.
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.
I believe this is the context key not a flag key. Which is why I left context key stuff alone. Correct me if I'm wrong @k3llymariee
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.
Got it. Makes sense.
Adds the remaining hardcoded SDK instructions, based off of these gonfalon files. A followup PR will remove the extra logic we have that decides whether to fetch or read instructions.