bcgov / namerequest

Public Front End for the Name Request System
1 stars 44 forks source link

16638 & 16639 Created new BulletsColinLink component + implemented new registration wireframes #620

Closed JazzarKarim closed 1 year ago

JazzarKarim commented 1 year ago

Issue #: bcgov/entity#16638 & bcgov/entity#16639

Description of changes:

Update:

Note: These changes implement the new registration wireframes in the UXPin (https://preview.uxpin.com/a86dccdd20ac62828f965c5eea10f81b828a9853#/pages/163136223/simulate/sitemap)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the namerequest license (Apache 2.0).

JazzarKarim commented 1 year ago

/gcbrun

pwei1018 commented 1 year ago

Temporary Url for review: https://namerequest-dev--pr-620-gvxwe8bm.web.app

JazzarKarim commented 1 year ago

/gcbrun

pwei1018 commented 1 year ago

Temporary Url for review: https://namerequest-dev--pr-620-gvxwe8bm.web.app

seeker25 commented 1 year ago

Lookin good Karim

JazzarKarim commented 1 year ago

Lookin good Karim

Thanks Travis!!

severinbeauvais commented 1 year ago

It would be nice if the designation went to the next line on a narrow screen:

image

severinbeauvais commented 1 year ago

Do you have guidance from UX to capitalize Name and Designation? If not then please follow the uxpins.

Original screenshot: image

Update: thanks for updating this.

severinbeauvais commented 1 year ago

~Bullet font size looks too large (compared with uxpin), and I think UX team wants the left block vertically centered with the right block.~ Update: I see you fixed this. Looks good!

image

severinbeauvais commented 1 year ago

"Incorporate Now" redirected me to login, but the return to Namerequest UI did not continue my request -- check App.vue.

Once I was logged in, Incorporate Now created a draft filing and redirected me to the business dashboard (as expected), but the entity type was wrong: I had selected a Limited Company but my draft IA is for a BEN. (This worked correctly for a ULC.)

Please confirm this, but I think Benefit Company should be BC and Limited Company should be CR.

Update: I commented on missing session storage code below. Once fixed, this comment can be ignored.

severinbeauvais commented 1 year ago

One time I tried to incorporate now, the API gave a 503 error, but no error was displayed in Namerequest UI. Please check that errors are correctly caught.

image

Update: I commented on unhandled error below. Once fixed, this comment can be ignored.

JazzarKarim commented 1 year ago

Looks really good (and works, too)! ๐Ÿ˜ƒ

Is there any chance you could create 2 unit tests: one to verify the display of the search page for a named company (showing Check this Name button), and for a numbered company (showing Incorporate Now button)?

Thanks Sev!

I have fixed most of the comments but there are still a few left. I am still getting the 503 error due an Axios Error sometimes. I need to fix that in addition to some styling and unit tests.

JazzarKarim commented 1 year ago

/gcbrun

pwei1018 commented 1 year ago

Temporary Url for review: https://namerequest-dev--pr-620-gvxwe8bm.web.app

JazzarKarim commented 1 year ago

Latest commit:

JazzarKarim commented 1 year ago

/gcbrun

pwei1018 commented 1 year ago

Temporary Url for review: https://namerequest-dev--pr-620-gvxwe8bm.web.app

JazzarKarim commented 1 year ago

Temporary Url for review: https://namerequest-dev--pr-620-gvxwe8bm.web.app

~For some reason, I can't see the feature flags in the temporary URL.~ It's working now.

JazzarKarim commented 1 year ago

/gcbrun

pwei1018 commented 1 year ago

Temporary Url for review: https://namerequest-dev--pr-620-gvxwe8bm.web.app

severinbeauvais commented 1 year ago
  • Created new dialog: IncorporateNowErrorDialog. Show this dialog when user incorporate now fails after user signs in. This also handles the error

I think there is only 1 error dialog shown app-wide, so here you are establishing a new pattern. Check with the UX team on this. (Originally there were more and more detailed errors displayed but the UX team wanted them simplified. I'm not sure giving detail here is appropriate.)

On the other hand, if your dialog is generic enough, could it be reused for the "firm register now" code?

  • Added decoy code to Signin.vue

"Decoy pattern" is not something you want to do, as it makes life harder for future devs. I'd rather see unused code deleted :) Were you able to confirm whether the code in Signin is ever used?

JazzarKarim commented 1 year ago
  • Created new dialog: IncorporateNowErrorDialog. Show this dialog when user incorporate now fails after user signs in. This also handles the error

I think there is only 1 error dialog shown app-wide, so here you are establishing a new pattern. Check with the UX team on this. (Originally there were more and more detailed errors displayed but the UX team wanted them simplified. I'm not sure giving detail here is appropriate.)

On the other hand, if your dialog is generic enough, could it be reused for the "firm register now" code?

  • Added decoy code to Signin.vue

"Decoy pattern" is not something you want to do, as it makes life harder for future devs. I'd rather see unused code deleted :) Were you able to confirm whether the code in Signin is ever used?

JazzarKarim commented 1 year ago
  • I will check with the UX team if it's OK. I was thinking of the best way to handle the errors and this seemed the nicest way. I'll confirm with the UX team however. Also, yes I believe the dialog should be reusable for the register now code for firms.

Just confirmed with the Janis. The dialog is OK. She said it's fine. However, I'm going to make a little adjustment to it.

severinbeauvais commented 1 year ago
  • Regarding the decoy pattern, I don't see the code for Signin.vue being used in the context of this PR. I tried debugging and testing by logging in and out many times. The code there is not being executed. Shall I, in this case, remove the unused code there?

If you're certain it's not used then, yes, unused (decoy) code should be deleted.

I believe Signin.vue is used by the login button in the header. Please try that. I'll try to confirm as well.

severinbeauvais commented 1 year ago

I believe Signin.vue is used by the login button in the header. Please try that. I'll try to confirm as well.

Confirmed. If you are in Namerequest UI, not logged in, and you login, then Signin.vue is used and onReady() is called.

However, it seems weird to me that we would need to check whether to restore a NR in session storage here. That's done in App.vue, isn't it? Or maybe it is done here and the code in App.vue isn't used? You'll need to test various scenarios to see which code is used and which isn't. I'm pretty sure we don't need both.

JazzarKarim commented 1 year ago

If you're certain it's not used then, yes, unused (decoy) code should be deleted.

I believe Signin.vue is used by the login button in the header. Please try that. I'll try to confirm as well.

Confirmed, you're right Sev ๐Ÿ˜„ Apologies for that, I missed it. I'll be leaving the code in Signin.vue.

However, I don't see a scenario where we need to restore the legal type from session storage after signing in from the header. That's why I'm leaning more to removing it.

JazzarKarim commented 1 year ago

However, it seems weird to me that we would need to check whether to restore a NR in session storage here. That's done in App.vue, isn't it? Or maybe it is done here and the code in App.vue isn't used? You'll need to test various scenarios to see which code is used and which isn't. I'm pretty sure we don't need both.

Yes, I totally agree.

severinbeauvais commented 1 year ago

However, I don't see a scenario where we need to restore the legal type from session storage after signing in from the header. That's why I'm leaning more to removing it.

Exactly. I believe the only time we put that data in session storage is to programmatically get them to login, and when we return to the app then App.vue continues the affiliation/draft. Conversely, if the user decides to login at some point, there shouldn't be that data in session storage.

Anyways, you got this. Clean up if possible. Thanks.

JazzarKarim commented 1 year ago

Exactly. I believe the only time we put that data in session storage is to programmatically get them to login, and when we return to the app then App.vue continues the affiliation/draft. Conversely, if the user decides to login at some point, there shouldn't be that data in session storage.

My thoughts exactly. All right, sounds good Sev!

severinbeauvais commented 1 year ago

It would be nice if the designation went to the next line on a narrow screen:

I see you've fixed this ๐Ÿ‘

image

JazzarKarim commented 1 year ago

/gcbrun

pwei1018 commented 1 year ago

Temporary Url for review: https://namerequest-dev--pr-620-gvxwe8bm.web.app

JazzarKarim commented 1 year ago

/gcbrun

pwei1018 commented 1 year ago

Temporary Url for review: https://namerequest-dev--pr-620-gvxwe8bm.web.app

JazzarKarim commented 1 year ago

@severinbeauvais In my latest commit, I moved the error status of incorporate now to the store. This is linked to the error dialog.

Reason: I want to show the error dialog whenever I want. This provides much more flexibility when we want to show the dialog. I suspect this is going to be helpful also when working on the other wireframes. Is that OK with you?

severinbeauvais commented 1 year ago

@severinbeauvais In my latest commit, I moved the error status of incorporate now to the store. This is linked to the error dialog.

Reason: I want to show the error dialog whenever I want. This provides much more flexibility when we want to show the dialog. I suspect this is going to be helpful also when working on the other wireframes. Is that OK with you?

OK with me! I appreciate the thought you put into this. Looks good.

JazzarKarim commented 1 year ago

/gcbrun

pwei1018 commented 1 year ago

Temporary Url for review: https://namerequest-dev--pr-620-gvxwe8bm.web.app

JazzarKarim commented 1 year ago

@severinbeauvais Please let me know if there are any outstanding comments Sev. Sorry about that, I know this work has been going on for a while now.

JazzarKarim commented 1 year ago

/gcbrun

pwei1018 commented 1 year ago

Temporary Url for review: https://namerequest-dev--pr-620-gvxwe8bm.web.app

JazzarKarim commented 1 year ago

/gcbrun