-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
dwkit frontend support #262
Conversation
59b6a88
to
80fcf60
Compare
49fbf15
to
40fccd2
Compare
2d87862
to
70f4f1b
Compare
…st Works ™️ --- now... how do I generate it myself upon JWT auth? Or... how do I configure the user claims from JWT to be what the cookie auth expects?
…wkit stuff is a massive abuse of payload size
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 SIL.AppBuilder.Portal.Frontend/src/public/assets/vendor/dwkit/ duplicates of OptimaJet.DWKit.StarterApplication/wwwroot/ or have they been changed? If changed, how will these be maintained on DWKit upgrades?
options.DefaultChallengeScheme = JwtBearerDefaults.AuthenticationScheme; | ||
}).AddJwtBearer(options => | ||
services.AddAuthentication() | ||
.AddJwtBearer(options => |
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.
How do these changes in authentication affect the DWKit Admin panel usage in development? At some point, the DWKit /admin
interface will need to be available to SuperAdmin (can be /dwkit
or something else).
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 can still login and pretend to know what I'm doing in the admin area before I realize I'll just break stuff. :)
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.
for the dwkit admin, I think we may just want a link from the Scriptoria admin to the URL that the dwkit admin is.
It could be like dwkit.scriptoria.io/admin
or something like that.
I don't trust OptimaJet's components to be flexible enough to handle be mounting on different URLs.
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.
👍
…ects are finished being setup
@@ -0,0 +1,17 @@ | |||
root = 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.
Don't we have this file at the root path?
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's the root of whatever folder you open your editor in
import { query, buildOptions, UserTaskResource } from '@data'; | ||
|
||
interface IOptions { | ||
include?: string; |
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.
include what? better to have more context. Also this one is string... name suggest boolean
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.
include is a keyword is jsonapi, it means, include relationships, it's literally what it says: https://jsonapi.org/format/#fetching-includes
|
||
return WrappedComponent => { | ||
function mapNetworkToProps(passedProps: TWRappedProps & IProps) { | ||
const { |
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.
Remove empty const {}
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.
thanks!
interface IProps {} | ||
|
||
export function withNetwork<TWRappedProps>(options: IOptions = {}) { | ||
const { include } = options; |
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 never used
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 is now :)
try { | ||
errorJson = await tryParseJson(response); | ||
} catch (e) { | ||
// body is not json |
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 console.warning? just swallow the error?
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 point.
@@ -0,0 +1,13 @@ | |||
import { AttributesObject, ResourceObject } from 'jsonapi-typescript'; |
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.
did you export this model in the data/index.tsx file?
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.
did you look? :P
(yes)
try { | ||
notify.show(msg, options.type, options.timeout, options.color); | ||
} catch (e) { | ||
console.log('Somthing horrible happened', e); |
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.
typo Somthing...
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.
thanks
export interface IYieldedProps { | ||
store: Store; | ||
} | ||
|
||
export interface IProps { |
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.
does it need to be exported?
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's not used, so it can be removed
@@ -1,3 +1,4 @@ | |||
export { default as PageError } from './page'; | |||
export { parseError } from './parse-error'; | |||
export { default as ErrorMessage } from './header-message'; |
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.
why not do export ErrorMessage from './header-message'
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.
because it doesn't have a named export, and the default export, by convention, is the default thing the file defines
include: [ | ||
'products.product-artifacts', | ||
'organization.organization-product-definitions.product-definition', | ||
GROUP, |
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.
never liked the idea of includes with variables.
GROUP => 'group' ... especially if they are just hiding strings... I suggest we change it to just regular text.
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.
variables are better, they are just a little misused at the moment.
the goal is to reduce misspellings, and catch errors at build time, rather than randomly at runtime.
we need a better solution for dot notation though.
export class App extends React.Component { | ||
constructor(props) { | ||
super(props); | ||
console.log('jQuery', $); |
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.
remove console
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.
done
}; | ||
|
||
const state = Store.getState(); | ||
console.log(Store, state); |
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.
same 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.
done
const state = Store.getState(); | ||
console.log(Store, state); | ||
|
||
let user = state.app.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.
it's better do let user = state.app.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.
not my code.
options.DefaultChallengeScheme = JwtBearerDefaults.AuthenticationScheme; | ||
}).AddJwtBearer(options => | ||
services.AddAuthentication() | ||
.AddJwtBearer(options => |
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.
👍
Right now, I can just Save and go back to tasks