ory / examples

A curated collection of examples and solutions created and maintained by the Ory Community.
https://www.ory.sh/community/
Apache License 2.0
135 stars 67 forks source link

fix: hydra kratos align ui version #71

Closed Jorgagu closed 1 year ago

Jorgagu commented 1 year ago

Align kratos-hydra example ui version and code to ory/kratos-selfservice-ui-node original project.

Related Issue or Design Document

Not a issue listed on the repo, but @mattbarnicle found that the design and text seems to not have the right style. Screenshot here : Kratos Seftservice UI Node with the wrong style

So I update the code used in the example to corresponding from ory/kratos-selfservice-ui-node and now we have the good font and style. Screenshot here: Kratos Seftservice UI Node with the good style

Checklist

Further comments

mattbarnicle commented 1 year ago

@Jorgagu why was the format chore necessary? i looked over it and it looks like a bunch of lines moved around. it adds a lot of noise to the overall PR.

Jorgagu commented 1 year ago

@Jorgagu why was the format chore necessary? i looked over it and it looks like a bunch of lines moved around. it adds a lot of noise to the overall PR.

Yes, this is a requirement of the contribution guidelines which you can find here: Code style

mattbarnicle commented 1 year ago

@Jorgagu why was the format chore necessary? i looked over it and it looks like a bunch of lines moved around. it adds a lot of noise to the overall PR.

Yes, this is a requirement of the contribution guidelines which you can find here: Code style

oh, this is just the result from the make format command? i ran that myself when i did the initial commit and it didn't move any lines around. i'm a bit confused, but if that's all this is, ok.

Jorgagu commented 1 year ago

@mattbarnicle I don't think you understand why and what I did on this PR. I just aligned all the code to the current state of the original kratos selfservice ui node project. I know it's not the same copyright year, but I just took the project and lined it up with this one, so I don't think your reviews are helpful here. But @vinckr needs to look at this PR and #67 today and he can tell if they are relevant or not.

mattbarnicle commented 1 year ago

@mattbarnicle I don't think you understand why and what I did on this PR. I just aligned all the code to the current state of the original kratos selfservice ui node project. I know it's not the same copyright year, but I just took the project and lined it up with this one, so I don't think your reviews are helpful here. But @vinckr needs to look at this PR and #67 today and he can tell if they are relevant or not.

ok, i see. i guess that's why a lot of code blocks seem to have been moved around also. well, Vincent asked me for help with the PR review, so that's what i'm trying to do. and FWIW, i do think my questions are valid ones, even if the code was merely integrated from another branch. perhaps the other branch isn't following the standards and needs to be updated too. but i suppose we can wait for Vincent's confirmation.

mattbarnicle commented 1 year ago

@Jorgagu i think i see what's going on now. when i created the initial commit on this branch from the code copied from the ory/kratos-selfservice-ui-node project, i copied in the Makefile from that project. but the examples project has its own Makefile in the root dir, which is the one that should be used when running make format. so the format target in Makefile in the kratos-hydra dir should probably be removed. and make format should be re-run from the project root dir. that will fix the copyright years and some of the other formatting issues i'm seeing, and should also get the PR to pass the Format job that it's currently failing on. also, perhaps other targets need to be taken out of that Makefile. i'm not sure about that part.

mattbarnicle commented 1 year ago

also, while clicking around on all the links from the Welcome screen, i clicked on the Account Verification link and noticed that the error flow isn't implemented. i added that locally to my kratos.yml file, but it might be better if you're going to update the PR to add that yourself. here's the diff:

diff --git a/kratos-hydra/contrib/hydra/kratos/kratos.yml b/kratos-hydra/contrib/hydra/kratos/kratos.yml
index 2653b29..bb967cd 100644
--- a/kratos-hydra/contrib/hydra/kratos/kratos.yml
+++ b/kratos-hydra/contrib/hydra/kratos/kratos.yml
@@ -40,6 +40,9 @@ selfservice:
           hooks:
             - hook: session

+    error:
+      ui_url: http://127.0.0.1:3000/error
+
 log:
   level: debug
Jorgagu commented 1 year ago

@mattbarnicle i have push the change you suggested.

mattbarnicle commented 1 year ago

thanks @vinckr!