Skip to content
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.

Sign up for GitHub

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

Jump to bottom

16637 Don't show numbered flow for CP + variable Incorporate Now button label #639

Merged
merged 9 commits into from
Aug 4, 2023
Merged

16637 Don't show numbered flow for CP + variable Incorporate Now button label #639

merged 9 commits into from
Aug 4, 2023

Conversation

ketaki-deodhar
Copy link
Collaborator

@ketaki-deodhar ketaki-deodhar commented Aug 3, 2023 •
edited
Loading

Issue #: /bcgov/entity#16637

Description of changes:

  • For Coop don't show numbered.
  • Re-use the existing feature flag for link to COLIN (until BC/CCC/ULC are released).
  • Verify remaining flow.
  • Trying to add/fix unit tests with no luck yet. Still looking into it
  • Use FF to determine whether to redirect to COLIN or Continue In Now

Temp url: https://namerequest-dev--pr-639-c9rqzr4l.web.app/

Some screenshots showing the Continuation In flow:

COOP:

image

image

Limited company Named:

image

image

Approved NR Limited company Named:

image

Limited company Numbered: Will always show link to Colin as Continuation In applications are not yet implemented

image

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).

@ketaki-deodhar ketaki-deodhar self-assigned this Aug 3, 2023
@ketaki-deodhar
Copy link
Collaborator Author

/gcbrun

@pwei1018
Copy link
Collaborator

pwei1018 commented Aug 3, 2023

Temporary Url for review: https://namerequest-dev--pr-639-c9rqzr4l.web.app

@ketaki-deodhar ketaki-deodhar marked this pull request as ready for review August 3, 2023 21:38
@severinbeauvais
Copy link
Collaborator

When the NR is approved, what does the Existing NR page look like for your Continuation In examples above?

@severinbeauvais severinbeauvais changed the title 16637 16637 Don't show numbered flow for CP + variable Incorporate Now button label Aug 3, 2023
@ketaki-deodhar
Copy link
Collaborator Author

ketaki-deodhar commented Aug 4, 2023 •
edited
Loading

@severinbeauvais this is how approved NR looks. It does not show 'Incorporate Now' button.

Approved NR Limited company Named:

image

supported entity check:
image
image

@severinbeauvais
Copy link
Collaborator

Thanks for the approved Continuation In NR. Should it should a link to COLIN?

@ketaki-deodhar
Copy link
Collaborator Author

Thanks for the approved Continuation In NR. Should it should a link to COLIN?

I looked in the code and there aren't any instances we show COLIN link in this component but definitely can be added. Should I check with Yui/Janis?

@severinbeauvais
Copy link
Collaborator

severinbeauvais commented Aug 4, 2023 •
edited by ketaki-deodhar
Loading

Thanks for the approved Continuation In NR. Should it should a link to COLIN?

I looked in the code and there aren't any instances we show COLIN link in this component but definitely can be added. Should I check with Yui/Janis?

We just decided this morning: no button to COLIN. (Same thing we have now.)

I will add related comments to the ticket regarding this

@ketaki-deodhar
Copy link
Collaborator Author

/gcbrun

@pwei1018
Copy link
Collaborator

pwei1018 commented Aug 4, 2023

Temporary Url for review: https://namerequest-dev--pr-639-c9rqzr4l.web.app

severinbeauvais
severinbeauvais approved these changes Aug 4, 2023
View reviewed changes
Copy link
Collaborator

@severinbeauvais severinbeauvais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but please get another review or 2 before merging. (You could ask Travis to add their new dev to this PR.)

@ketaki-deodhar
Copy link
Collaborator Author

LGTM but please get another review or 2 before merging. (You could ask Travis to add their new dev to this PR.)

as a reviewer?

@severinbeauvais
Copy link
Collaborator

severinbeauvais commented Aug 4, 2023 •
edited
Loading

LGTM but please get another review or 2 before merging. (You could ask Travis to add their new dev to this PR.)

as a reviewer?

Yes, but I'm mixed up. This is Namerequest. That other dev is working in Auth Web.

I'll ask Eve. She likes Namerequest I think 😄

@ketaki-deodhar
Copy link
Collaborator Author

ketaki-deodhar commented Aug 4, 2023 •
edited
Loading

@seeker25 can you please add your new dev to this PR?

@seeker25 Disregard ^^

chenhongjing
chenhongjing approved these changes Aug 4, 2023
View reviewed changes
Copy link
Collaborator

@chenhongjing chenhongjing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐱

@severinbeauvais severinbeauvais merged commit 0c55ba0 into bcgov:feature-way-of-navigating Aug 4, 2023
5 checks passed
@seeker25
Copy link
Collaborator

seeker25 commented Aug 4, 2023

LGTM

JazzarKarim pushed a commit to JazzarKarim/namerequest that referenced this pull request Aug 24, 2023
@ketaki-deodhar @JazzarKarim
16637 Don't show numbered flow for CP + variable Incorporate Now butt…
b10cd3c 
…on label (bcgov#639)

* 16637-Continuation in flow 1

* 16637-Continuation in flow 2

* 16637-update package version

* 16637-fix css class and remove unused css

* 16637-remove unused css

* 16637-update package version
JazzarKarim pushed a commit to JazzarKarim/namerequest that referenced this pull request Aug 24, 2023
@ketaki-deodhar @JazzarKarim
16637 Don't show numbered flow for CP + variable Incorporate Now butt…
8cbb1fc 
…on label (bcgov#639)

* 16637-Continuation in flow 1

* 16637-Continuation in flow 2

* 16637-update package version

* 16637-fix css class and remove unused css

* 16637-remove unused css

* 16637-update package version
JazzarKarim pushed a commit to JazzarKarim/namerequest that referenced this pull request Sep 8, 2023
@ketaki-deodhar @JazzarKarim
16637 Don't show numbered flow for CP + variable Incorporate Now butt…
b1771ef 
…on label (bcgov#639)

* 16637-Continuation in flow 1

* 16637-Continuation in flow 2

* 16637-update package version

* 16637-fix css class and remove unused css

* 16637-remove unused css

* 16637-update package version
JazzarKarim pushed a commit that referenced this pull request Sep 11, 2023
@ketaki-deodhar @JazzarKarim
16637 Don't show numbered flow for CP + variable Incorporate Now butt…
179666e 
…on label (#639)

* 16637-Continuation in flow 1

* 16637-Continuation in flow 2

* 16637-update package version

* 16637-fix css class and remove unused css

* 16637-remove unused css

* 16637-update package version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@severinbeauvais severinbeauvais severinbeauvais approved these changes

@eve-git eve-git eve-git approved these changes

@chenhongjing chenhongjing chenhongjing approved these changes

@JazzarKarim JazzarKarim Awaiting requested review from JazzarKarim

@bennypc bennypc Awaiting requested review from bennypc

@leodube-aot leodube-aot Awaiting requested review from leodube-aot

Assignees

@ketaki-deodhar ketaki-deodhar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ketaki-deodhar @pwei1018 @severinbeauvais @seeker25 @eve-git @chenhongjing