-
Notifications
You must be signed in to change notification settings - Fork 3
chore: process all changes for demo #39
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
Conversation
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
Co-authored-by: woutslabbinck <wout.slabbinck@ugent.be>
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
…-access into demo/setup
woutslabbinck
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.
There is the fact that npm start:all does end up in the css failing. I think this should be fixed or made an issue.
Furthermore, I've made some small comments that IMO should be resolved before we merge into main
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.
Is these and all other .internal data required?
If so, why exactly again?
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 internal data keeps track of the accounts related to the pods. I think it's best to keep those, to be able to use the CSS account system when needed. For example, when we want to extend the demo with a real login using the CSS's IDP.
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.
What does this do?
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 a meta file the CSS automatically adds in each pod.
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 meant, what does the catalog do again?
demo/data/demo/public/authorizationsite/public/example_policy.jsonld
Outdated
Show resolved
Hide resolved
demo/data/demo/public/authorizationsite/public/example_policy.nq
Outdated
Show resolved
Hide resolved
|
@woutslabbinck, thanks for the thorough review! I replied to some comments, and gave thumbs up to those I'll fix. A few other questions are for @Dexagod to answer. |
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
|
I refactored the demo sites a bit, moving them out of the css, and trimming some of the fat of the react installation. @Dexagod can you take a look if you are okay with those changes? (For ease of installation, I added a global |
|
I'll go over the demo changes |
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
f54817a to
2309560
Compare
|
Alright, so now 2309560 takes care of the timing problems with the parallel start. |
Signed-off-by: Wouter Termont <wouter.termont@ugent.be>
|
I believe all your comments have been addressed, @woutslabbinck, so give it another look if you have some time. Thanks! |
This PR merges the
demo/setupbranch into main. I'll try to rebase it into a few comprehensible chunks of work.@Dexagod, will you push the demo apps to this branch before merging, or to another branch that we can merge later?