-
Notifications
You must be signed in to change notification settings - Fork 320
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: Adds Migrating file #656
docs: Adds Migrating file #656
Conversation
MIGRATING.md
Outdated
|
||
### Moved dependencies to devDependency | ||
|
||
This should probably not affect your project, but we moved `okta-auth-js/jquery` and `backbone` from `dependecies` to `devDependency`. |
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.
okta-auth-js
MIGRATING.md
Outdated
|
||
If you have questions about this library or about the Okta APIs, post a question on our [Developer Forum](https://devforum.okta.com). | ||
|
||
If you find a bug or have a feature request for this library specifically, [post an issue](https://github.com/okta/okta-sdk-dotnet/issues) here on GitHub. |
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.
copy/parse typo ;)
https://github.com/okta/okta-signin-widget/issues
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.
ahahah good catch!
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 to me
39b0fa4
to
46b2941
Compare
MIGRATING.md
Outdated
@@ -0,0 +1,84 @@ | |||
# Okta Signin Widget migration guide |
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.
nit: Signin -> Sign-In
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.
nitnit: Okta Sign-in Widget migration guide
MIGRATING.md
Outdated
|
||
## Migrating from 2.x to 3.x | ||
|
||
### Consolidated css files to single one |
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.
suggestion: Remove "to single one" to be more concise:
Consolidated CSS files
MIGRATING.md
Outdated
|
||
### Consolidated css files to single one | ||
|
||
In version 2.x we had two separate css files to import: `okta-sign-in.min.css` and `okta-theme.css`. We moved to have one single css file which is still called `okta-sign-in.min.css`. |
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.
In version 2.x we had two separate css files to import: `okta-sign-in.min.css` and `okta-theme.css`. We moved to have one single css file which is still called `okta-sign-in.min.css`. | |
In version 2.x two separate css files were required to import: `okta-sign-in.min.css` and `okta-theme.css`. Now, `okta-sign-in.min.css` is the only available css import. |
```html | ||
<!-- BEFORE --> | ||
<link | ||
href="https://ok1static.oktacdn.com/assets/js/sdk/okta-signin-widget/2.14.0/css/okta-sign-in.min.css" |
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.
Curious if you think we should be more generic with the version, and say /okta-signin-widget/{version}/css/..
?
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.
Since it's an actual example and I wanted to explicitly show passing from 2.x to 3.x, I opted for showing a real case.
MIGRATING.md
Outdated
}); | ||
``` | ||
|
||
### Moved dependencies to devDependency |
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.
Since this shouldn't impact developers, I suggest removing it from our migration guide.
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.
Make sense, @haishengwu-okta what do you think?
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.
sounds good to remove 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.
I agree, we don't need to call these out. No code changes required.
|
||
### Renamed functions | ||
|
||
`tokenManager.refresh` has been renamed to `tokenManager.renew`, so you should update it in your 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.
nit: Let's keep the signIn
object prefix to be more verbose:
`tokenManager.refresh` has been renamed to `tokenManager.renew`, so you should update it in your code. | |
`signIn.tokenManager.refresh` has been renamed to `signIn.tokenManager.renew`, so you should update it in your 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.
The developer could name the variable whatever they want, it isn't always signIn
. I'd prefer to drop the prefix to stay in line with our API docs: https://github.com/okta/okta-signin-widget/#tokenmanagerrefresh
MIGRATING.md
Outdated
|
||
`tokenManager.refresh` has been renamed to `tokenManager.renew`, so you should update it in your code. | ||
|
||
### Token retrieval is now asyncronous to account for automatic token renewal |
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.
nit: Shorter titles are better 😄
### Token retrieval is now asyncronous to account for automatic token renewal | |
### Token retrieval is now asynchronous | |
MIGRATING.md
Outdated
|
||
### Token retrieval is now asyncronous to account for automatic token renewal | ||
|
||
`signIn.tokenManager.get('accessToken')` moved to be synchronous in 2.x to asynchronous in 3.x |
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.
nit: .
at the end.
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.
Suggested:
Starting in version 3.0,
tokenManager.get
is an asynchronous function. It returns an object you can handle as a promise:
MIGRATING.md
Outdated
@@ -0,0 +1,84 @@ | |||
# Okta Signin Widget migration guide |
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.
nitnit: Okta Sign-in Widget migration guide
MIGRATING.md
Outdated
rel="stylesheet"/> | ||
``` | ||
|
||
- If you were building your css files through `sass`, you will need to build it again and this time it will generate only `okta-sign-in.min.css` instead of the two files. |
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.
nit: No need to code-style sass
, it is "Sass".
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.
nit: Replace all "css" with "CSS"
|
||
### Renamed functions | ||
|
||
`tokenManager.refresh` has been renamed to `tokenManager.renew`, so you should update it in your 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.
The developer could name the variable whatever they want, it isn't always signIn
. I'd prefer to drop the prefix to stay in line with our API docs: https://github.com/okta/okta-signin-widget/#tokenmanagerrefresh
MIGRATING.md
Outdated
|
||
### Token retrieval is now asyncronous to account for automatic token renewal | ||
|
||
`signIn.tokenManager.get('accessToken')` moved to be synchronous in 2.x to asynchronous in 3.x |
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.
Suggested:
Starting in version 3.0,
tokenManager.get
is an asynchronous function. It returns an object you can handle as a promise:
MIGRATING.md
Outdated
}); | ||
``` | ||
|
||
### Moved dependencies to devDependency |
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 agree, we don't need to call these out. No code changes required.
MIGRATING.md
Outdated
|
||
In version 2.x we had two separate css files to import: `okta-sign-in.min.css` and `okta-theme.css`. We moved to have one single css file which is still called `okta-sign-in.min.css`. | ||
|
||
Depending on your configuration, this is what you should change: |
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.
Depending on how you are using the Sign-in Widget in your application, here's what you need to do:
MIGRATING.md
Outdated
|
||
Depending on your configuration, this is what you should change: | ||
|
||
- If you were using CDN links for the css, you will need to change the `okta-sign-in.min.css` link and remove the `okta-theme.css` link. |
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.
"change" -> "update"
Added MIGRATING.md with instructions to migrate from v2.x to v3.x
6276097
to
c7e6548
Compare
c7e6548
to
94d50b2
Compare
Added MIGRATING.md with instructions to migrate from v2.x to v3.x
Added MIGRATING.md with instructions to migrate from v2.x to v3.x
Added MIGRATING.md with instructions to migrate from v2.x to v3.x
Added MIGRATING.md with instructions to migrate from v2.x to v3.x
Added MIGRATING.md with instructions to migrate from v2.x to v3.x
Added MIGRATING.md with instructions to migrate from v2.x to v3.x
Added MIGRATING.md with instructions to migrate from v2.x to v3.x
Added MIGRATING.md with instructions to migrate from v2.x to v3.x
Added MIGRATING.md with instructions to migrate from v2.x to v3.x
Added MIGRATING.md with instructions to migrate from v2.x to v3.x
Description
Added MIGRATING.md with instructions to migrate from v2.x to v3.x
Reviewers
@robertdamphousse-okta @nbarbettini @haishengwu-okta @jmelberg-okta