25157 - close the comment modal after saving comments/details #131

patrickpeinanw · 2025-01-23T22:23:59Z

Description of changes:

We use the same modal to add comments and details. The modal does not close when the 'save' button is clicked. The issue is fixed in this PR

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

patrickpeinanw · 2025-01-23T23:02:57Z

cypress/e2e/components/filings/correctionFilings.cy.ts

-    cy.visitBusinessDashFor('businessInfo/ben/active.json', undefined, false, false, undefined, allFilings, true)
+    cy.visitBusinessDashFor(
+      'businessInfo/ben/active.json', undefined, false, false, undefined, [administrativeDissolution], true
+    )
    cy.intercept('POST', '**/api/v2/businesses/**/filings', {}).as('correctionFilingsPost')

    cy.get('[data-cy="header.actions.dropdown"]').should('exist')
-    //select filing 2 instead of 0 or 1 which are correctionable
-    cy.get('[data-cy="header.actions.dropdown"]').eq(2).click()
+
+    // open the dropdown for the administrative dissolution in the Filing History; correction option should be disabled
+    cy.get('[data-cy="header.actions.dropdown"]').first().click()


This test tends to fail on PR. It attempts to select the administrative dissolution filing in the Filing History and confirm its correction option is disabled. We load allFilings (cypress/fixtures/filings/allFilings.ts), and the filing we are interested in should be in the fourth position and can be selected by eq(3). But for some reason, it sometimes gets rendered as the third item so we have to use eq(2).

To avoid this mess. we can just specify the filing we want to render instead of loading all filings.

Thank you for fixing this!

JazzarKarim

LGTM 👍

severinbeauvais

I still can't test but here's my review:

The code looks fine.

I specifically want to test a business comment since Filings UI left that comment dialog open on save, but auto-closed the filing comments dialog on save.

If you want, merge this and test it in Dev. If the business comment now auto-closes then check with @jacqueline-williams-549 if that's OK, otherwise it will need to be changed.

severinbeauvais · 2025-01-23T23:51:32Z

By the way, are all PRs in this repo (and maybe business-ui) now untestable with the missing keys? That definitely needs to be fixed. Feel free to create a new ticket if it's taking a while to solve it.

JazzarKarim · 2025-01-23T23:52:45Z

By the way, are all PRs in this repo (and maybe business-ui) now untestable with the missing keys? That definitely needs to be fixed. Feel free to create a new ticket if it's taking a while to solve it.

Yep, I will have a PR up in a sec to fix this issue.

JazzarKarim

Patrick, can you please rebase your PR and do a gcbrun again? It should work now.

patrickpeinanw · 2025-01-24T00:49:26Z

/gcbrun

bcregistry-sre · 2025-01-24T00:52:09Z

Temporary Url for review: https://business-dashboard-dev--pr-131-qpworkah.web.app

SB says, try this: https://business-dashboard-dev--pr-131-qpworkah.web.app/FM1101299

Or this: https://business-dashboard-dev--pr-131-qpworkah.web.app/BC0885606

jacqueline-williams-549 · 2025-01-24T17:02:33Z

I still can't test but here's my review:

The code looks fine.

I specifically want to test a business comment since Filings UI left that comment dialog open on save, but auto-closed the filing comments dialog on save.

If you want, merge this and test it in Dev. If the business comment now auto-closes then check with @jacqueline-williams-549 if that's OK, otherwise it will need to be changed.

I noticed that it's happening in test. The dialogue doesn't close when you hit save. I didn't design this piece, but at first glance it seems intentional so that the user is able to see their comment added to the list of comments below. I don't have any serious concerns with the current functionality. I think the dialogue is easily closeable with the X icon. I can review with Janis for her opinion, but she's away til Monday.

Screen.Recording.2025-01-24.at.8.55.30.AM.mov

severinbeauvais · 2025-01-24T18:18:05Z

I noticed that it's happening in test. The dialogue doesn't close when you hit save. I didn't design this piece, but at first glance it seems intentional so that the user is able to see their comment added to the list of comments below. I don't have any serious concerns with the current functionality. I think the dialogue is easily closeable with the X icon. I can review with Janis for her opinion, but she's away til Monday.

There are separate actions to add comments: filing comments and business comments. In Filings UI, we had separate dialogs to add each comment. In Business Dashboard UI, it's the same dialog. In Filings UI, the filing comments dialog closed after saving because you could see the comments in the filing. In Filings UI, the business comments dialog stayed open so you could see your comments in the list (which is normally hidden).

So what do we want now -- same behaviour as Filings UI? Separate dialogs? Or keep the filing comments dialog displaying at the top left (where the business comments normally displays) (instead of in the middle of the page), and the dialog stays open and you have to close it explicitly?

severinbeauvais · 2025-01-24T18:23:10Z

Re: https://business-dashboard-dev--pr-131-qpworkah.web.app/FM1101299

What we have now is the dialog auto-closes for both business comments and filing comments. To me, this doesn't seem so bad. @jacqueline-williams-549 , please try it and see what you think.

Compare with https://business-filings-dev--pr-713-3ar5fu8y.web.app/FM1101299/?noRedirect=true

jacqueline-williams-549 · 2025-01-24T20:06:34Z

I noticed that it's happening in test. The dialogue doesn't close when you hit save. I didn't design this piece, but at first glance it seems intentional so that the user is able to see their comment added to the list of comments below. I don't have any serious concerns with the current functionality. I think the dialogue is easily closeable with the X icon. I can review with Janis for her opinion, but she's away til Monday.

There are separate actions to add comments: filing comments and business comments. In Filings UI, we had separate dialogs to add each comment. In Business Dashboard UI, it's the same dialog. In Filings UI, the filing comments dialog closed after saving because you could see the comments in the filing. In Filings UI, the business comments dialog stayed open so you could see your comments in the list (which is normally hidden).

So what do we want now -- same behaviour as Filings UI? Separate dialogs? Or keep the filing comments dialog displaying at the top left (where the business comments normally displays) (instead of in the middle of the page), and the dialog stays open and you have to close it explicitly?

I think it would be confusing to the user if hitting save does different things on 2 dialogs that look almost identical. ~~I think we should make it work the same for both Business Comments and Filing Comments: The user hits the X icon to close the dialog and hitting 'save' adds the comment to the list below.~~
Let's keep it as is - save closes the dialog in both cases. The number of comments for the business changes and the filing comments are displayed immediately when the dialog closes
Like the other dialogs and pop ups, I prefer it displays in the middle of the screen if possible, but it doesn't bother me much to have it on the side either
It would be nice eventually if we could make it clear while the dialog is open, that something is a business comment (3 Comments | Business Details) or a filing comment (3 Comments | Business Record) - see below. This requires approval by Janis or Design Team though, which is not possible until Tuesday. And is probably outside the scope of this ticket.

severinbeauvais · 2025-01-24T20:32:15Z

Let's keep it as is - save closes the dialog in both cases.

OK! Then this PR is good as-is. Ready to merge it, @patrickpeinanw ?

patrickpeinanw self-assigned this Jan 23, 2025

This comment was marked as outdated.

Sign in to view

patrickpeinanw commented Jan 23, 2025

View reviewed changes

patrickpeinanw marked this pull request as ready for review January 23, 2025 23:03

patrickpeinanw requested review from hfekete, severinbeauvais, JazzarKarim, Rajandeep98 and ketaki-deodhar as code owners January 23, 2025 23:03

This comment was marked as resolved.

Sign in to view

JazzarKarim approved these changes Jan 23, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

severinbeauvais approved these changes Jan 23, 2025

View reviewed changes

JazzarKarim reviewed Jan 24, 2025

View reviewed changes

patrickpeinanw added 3 commits January 23, 2025 16:47

25157 - close the comment modal after saving comments/details

e1a186b

update version

25fda15

update correctionFilings cypress test

53abd11

patrickpeinanw force-pushed the main branch from 547439f to 53abd11 Compare January 24, 2025 00:48

patrickpeinanw merged commit 8040ca0 into bcgov:main Jan 24, 2025
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

25157 - close the comment modal after saving comments/details #131

25157 - close the comment modal after saving comments/details #131

patrickpeinanw commented Jan 23, 2025

This comment was marked as outdated.

This comment was marked as outdated.

patrickpeinanw Jan 23, 2025 •

edited

Loading

severinbeauvais Jan 23, 2025

This comment was marked as resolved.

This comment was marked as resolved.

JazzarKarim left a comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as outdated.

severinbeauvais left a comment •

edited

Loading

severinbeauvais commented Jan 23, 2025

JazzarKarim commented Jan 23, 2025

JazzarKarim left a comment

patrickpeinanw commented Jan 24, 2025

bcregistry-sre commented Jan 24, 2025 •

edited by severinbeauvais

Loading

jacqueline-williams-549 commented Jan 24, 2025

severinbeauvais commented Jan 24, 2025 •

edited

Loading

severinbeauvais commented Jan 24, 2025

jacqueline-williams-549 commented Jan 24, 2025 •

edited

Loading

severinbeauvais commented Jan 24, 2025

25157 - close the comment modal after saving comments/details #131

25157 - close the comment modal after saving comments/details #131

Conversation

patrickpeinanw commented Jan 23, 2025

This comment was marked as outdated.

This comment was marked as outdated.

patrickpeinanw Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

severinbeauvais Jan 23, 2025

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

JazzarKarim left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as outdated.

severinbeauvais left a comment • edited Loading

Choose a reason for hiding this comment

severinbeauvais commented Jan 23, 2025

JazzarKarim commented Jan 23, 2025

JazzarKarim left a comment

Choose a reason for hiding this comment

patrickpeinanw commented Jan 24, 2025

bcregistry-sre commented Jan 24, 2025 • edited by severinbeauvais Loading

jacqueline-williams-549 commented Jan 24, 2025

severinbeauvais commented Jan 24, 2025 • edited Loading

severinbeauvais commented Jan 24, 2025

jacqueline-williams-549 commented Jan 24, 2025 • edited Loading

severinbeauvais commented Jan 24, 2025

patrickpeinanw Jan 23, 2025 •

edited

Loading

severinbeauvais left a comment •

edited

Loading

bcregistry-sre commented Jan 24, 2025 •

edited by severinbeauvais

Loading

severinbeauvais commented Jan 24, 2025 •

edited

Loading

jacqueline-williams-549 commented Jan 24, 2025 •

edited

Loading