EASI-4667 Side Panel Routing / State management #2901

adamodd · 2024-11-22T16:52:21Z

EASI-4667

Description

New hook useDiscussionParams to wrap url query handling.

Use new url search query params discussionMode and discussionId with modes for subview states. Example query strings

?discussionMode=view
?discussionMode=start
?discussionMode=reply&discussionId=63deffff-31be-49ca-9733-8247d0446cda

For the base path /it-governance/61efa6eb-1976-4431-a158-d89cc00ce31d/grb-review

Names and param format is not fixed and open for discussion.

Using query params is probably the way to go for the modal states of a higher-level view, instead of something such as appending further route paths (under grb.)

How to test this change

QA the interactions in the DiscussionBoard side panel

PR Author Checklist

I have provided a detailed description of the changes in this PR.
I have provided clear instructions on how to test the changes in this PR.
I have updated tests or written new tests as appropriate in this PR.

PR Reviewer Guidelines

It's best to pull the branch locally and test it, rather than just looking at the code online!
When approving a PR, provide a reason why you're approving it
- e.g. "Approving because I tested it locally and all functionality works as expected"
- e.g. "Approving because the change is simple and matches the Figma design"
Don't be afraid to leave comments or ask questions, especially if you don't understand why something was done! (This is often a great time to suggest code comments or documentation updates)
Check that all code is adequately covered by tests - if it isn't feel free to suggest the addition of tests.

…s' into EASI-4667/panel-query-state

ClayBenson94

It looks like the code to set the url params is working as expected, but it doesn't actually seem to be showing the side panel for me locally. Not sure if it's something on my machine specifically or not -- are you able to reproduce this?

adamodd · 2024-11-26T20:09:21Z

@ClayBenson94 Yikes! Missed a line change in the reorg. Should be good now, sorry!

ClayBenson94

Appreciate that quick change -- looks like it's handling the proper views based on the query params now!

2 things I noticed when testing it out:

I don't think we're refetching data on some of these forms, but that might be something that @aterstriep is handling with her form work (so we can ignore it here, if so!)
I think this Cancel button on the reply modal (src) should navigate back to the view mode, rather than close the modal. Could you update that to use pushDiscussionQuery? (Assuming that's the solution)
- I think this will also handle it for the start view as well?

adamodd · 2024-11-26T21:09:13Z

I think this Cancel button on the reply modal (src) should navigate back to the view mode

I'll have a look

adamodd · 2024-11-26T21:20:52Z

I don't think we're refetching data on some of these forms

I did not work on the original queries, but I did notice that ids do exist on the entities, which afaik is at least part of a possible path to the refetching behavior.

Cancel button in reply mode is updated. @ClayBenson94

adamodd · 2024-11-26T21:43:14Z

If the refetching issue isn't simple we may want to split that out to a NOREF since the diff for this commit is big enough, imo.

ClayBenson94 · 2024-11-26T21:44:05Z

If the refetching issue isn't simple we may want to split that out to a NOREF since the diff for this commit is big enough, imo.

Yeah, I don't think the refetching is necessarily something we need to solve here -- the query param handling / navigation is a good spot to break this one off I think, so as long as all the buttons/links send to the right spots I'm good!

…ionMode`

aterstriep · 2024-11-27T15:57:32Z

I don't think we're refetching data on some of these forms

I'm handling this in the PR I'll have up today to finish the discussion views work.

I'll have a review up on this soon!

aterstriep

Nice job on this! I just found a couple of issues with the success/error alerts after submitting the form.

Can you add some unit tests to cover navigating between the different panel views?

aterstriep · 2024-11-27T17:50:34Z

src/views/DiscussionBoard/components/DiscussionForm.tsx

@@ -77,15 +80,15 @@ const DiscussionForm = ({
            message: t('general.alerts.startDiscussionSuccess'),
            type: 'success'
          });
+
+          pushDiscussionQuery({ discussionMode: 'view' });


This needs to go under the mutation block where I had left a TODO comment before. On submit, the form should navigate back to the main discussion board view on both error and success (see Figma).

aterstriep · 2024-11-27T17:58:30Z

src/views/DiscussionBoard/index.tsx


  // Reset discussionAlert when side panel is opened or closed
  useEffect(() => {
    setDiscussionAlert(null);
-  }, [setDiscussionAlert, isOpen]);
+  }, [setDiscussionAlert, discussionMode]);


I see what you were trying to do here, but this discussionMode dependency is preventing the discussionAlert from showing after creating a new discussion.

Thanks for pointing this out

I'm actually not sure what should happen here with what you had originally + the new subview states.

The original comment says to reset on open or close, which was before the subview states. I used discussionMode because I figured the concept of open|close was similar enough to the mode subview switching, though the effect now is definitely wrong.

I test reverted to the isOpen idea, but the alert just stays up if the user decides to keep interacting with more convos without closing the panel. Not saying a revert is what you're suggesting. Just not sure of where to go with what's set up.

Any ideas on how to approach? @aterstriep

Ideally, the discussionAlert needs to be reset in two instances:

discussionMode changes from reply

discussionMode changes from view

If I remember right, Mint uses setTimeout to close their alerts after a certain time period. So alternatively we could do it that way as well as reset when the modal is opened or closed.

Happy to pair on this later today if that would be helpful!

aterstriep · 2024-11-27T18:00:40Z

src/hooks/useDiscussionParams.ts

Good idea to separate this code into a custom hook!

adamodd · 2024-11-27T18:12:26Z

Can you add some unit tests to cover navigating between the different panel views?

@aterstriep Weren't you planning to take on the cypress tests? I think that would cover testing.

adamodd · 2024-11-27T18:19:53Z

@aterstriep There was also the refetching issue that was pointed out up the thread. Seemed like it would have been part of your current follow-up work but not sure.

aterstriep · 2024-11-27T19:45:32Z

There was also the refetching issue that was pointed out up the thread. Seemed like it would have been part of your current follow-up work but not sure.

@adamodd I left a comment about this above but forgot to tag you. Yes, this will be handled in the follow-up work.

adamodd · 2024-11-27T19:57:05Z

@aterstriep Okay, let me know what you have in mind for unit tests in this PR.

I'm not clear on what it should be because the refetching issue is being handled in yours, and I thought I at least heard in passing that you would take on the cypress tests next.

We can unit test the state handling, but I think there's less to code with the cypress testing methods and might prefer that.

ClayBenson94 · 2024-12-02T14:05:39Z

Regarding Testing -- if the JS tests are simple to add in, I think it could be nice to have even just some snapshot tests that ensure things render as expected given certain props/state, even if we don't go all out with handling navigating between different views in the tests (and just save that for Cypress)

Maybe some tests that just ensure the right things are rendered based on discussionMode could be a good middle ground for testing?

aterstriep · 2024-12-03T15:52:37Z

I'm not clear on what it should be because the refetching issue is being handled in yours, and I thought I at least heard in passing that you would take on the cypress tests next.

We can unit test the state handling, but I think there's less to code with the cypress testing methods and might prefer that.

@adamodd I was thinking just some basic unit tests to make sure the right content is shown at each view, but I'm good with handling this in Cypress tests instead. The Cypress tests have been ticketed but we haven't discussed who would be taking it on.

@ClayBenson94 we can't use snapshots for the discussions work. Some of the dates are displayed as "X days ago", so the snapshots would change every day. I figured this out the hard way 😅

adamodd · 2024-12-03T18:20:30Z

@aterstriep @ClayBenson94 The alert issue is fixed. That looked like the last outstanding item.

Tests will continue in the separate ticket. There were also some unit tests for renders already done.

aterstriep

Changes look good!

DiscussionMode query handling

daee6ef

adamodd temporarily deployed to test November 22, 2024 16:52 — with GitHub Actions Inactive

adamodd changed the base branch from main to feature/EASI-4614_grb_discussions November 22, 2024 16:54

Update tests with MemoryRouter

a23bb53

adamodd temporarily deployed to test November 22, 2024 17:20 — with GitHub Actions Inactive

adamodd added 5 commits November 25, 2024 11:35

Merge remote-tracking branch 'origin/feature/EASI-4614_grb_discussion…

d9de7ff

…s' into EASI-4667/panel-query-state

Fix closure typo

e49a89a

DiscussionList keys

c322fa9

Create new, submit, go to discussions view

63f4778

Open to start discussion view

0418069

adamodd temporarily deployed to test November 25, 2024 20:05 — with GitHub Actions Inactive

useDiscussion hook

d1a915c

adamodd temporarily deployed to test November 26, 2024 00:46 — with GitHub Actions Inactive

discussionId determines activeDiscussion

8ab44ca

adamodd temporarily deployed to test November 26, 2024 16:22 — with GitHub Actions Inactive

adamodd added 2 commits November 26, 2024 11:32

useDiscussionParams reorganize

5c4761c

Merge remote-tracking branch 'origin/feature/EASI-4614_grb_discussion…

7bb20a4

…s' into EASI-4667/panel-query-state

adamodd temporarily deployed to test November 26, 2024 19:33 — with GitHub Actions Inactive

adamodd marked this pull request as ready for review November 26, 2024 19:39

adamodd requested a review from a team as a code owner November 26, 2024 19:39

adamodd requested review from aterstriep and removed request for a team November 26, 2024 19:39

ClayBenson94 reviewed Nov 26, 2024

View reviewed changes

Oops!

21a7ff5

adamodd temporarily deployed to test November 26, 2024 20:08 — with GitHub Actions Inactive

ClayBenson94 requested changes Nov 26, 2024

View reviewed changes

DiscussionForm -> Cancel -> Back to view mode

8c145ec

adamodd temporarily deployed to test November 26, 2024 21:17 — with GitHub Actions Inactive

adamodd requested a review from ClayBenson94 November 26, 2024 21:20

Keep extracted reference instead of redefining literals with `Discuss…

b439319

…ionMode`

adamodd temporarily deployed to test November 27, 2024 00:44 — with GitHub Actions Inactive

Rename useDiscussionParams.ts

7d21f62

adamodd temporarily deployed to test November 27, 2024 00:48 — with GitHub Actions Inactive

aterstriep requested changes Nov 27, 2024

View reviewed changes

Finally push

8041eb7

adamodd temporarily deployed to test November 27, 2024 22:05 — with GitHub Actions Inactive

Reset alert on mode change

8875d14

adamodd temporarily deployed to test December 3, 2024 18:17 — with GitHub Actions Inactive

adamodd requested a review from aterstriep December 3, 2024 18:18

aterstriep approved these changes Dec 3, 2024

View reviewed changes

adamodd merged commit 969297f into feature/EASI-4614_grb_discussions Dec 3, 2024
12 checks passed

adamodd deleted the EASI-4667/panel-query-state branch December 3, 2024 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EASI-4667 Side Panel Routing / State management #2901

EASI-4667 Side Panel Routing / State management #2901

adamodd commented Nov 22, 2024 •

edited

Loading

ClayBenson94 left a comment

adamodd commented Nov 26, 2024

ClayBenson94 left a comment

adamodd commented Nov 26, 2024

adamodd commented Nov 26, 2024 •

edited

Loading

adamodd commented Nov 26, 2024

ClayBenson94 commented Nov 26, 2024

aterstriep commented Nov 27, 2024

aterstriep left a comment

aterstriep Nov 27, 2024

aterstriep Nov 27, 2024

adamodd Nov 27, 2024

adamodd Nov 27, 2024

aterstriep Dec 3, 2024

aterstriep Nov 27, 2024

adamodd commented Nov 27, 2024

adamodd commented Nov 27, 2024 •

edited

Loading

aterstriep commented Nov 27, 2024 •

edited

Loading

adamodd commented Nov 27, 2024

ClayBenson94 commented Dec 2, 2024

aterstriep commented Dec 3, 2024

adamodd commented Dec 3, 2024

aterstriep left a comment

EASI-4667 Side Panel Routing / State management #2901

EASI-4667 Side Panel Routing / State management #2901

Conversation

adamodd commented Nov 22, 2024 • edited Loading

EASI-4667

Description

How to test this change

PR Author Checklist

PR Reviewer Guidelines

ClayBenson94 left a comment

Choose a reason for hiding this comment

adamodd commented Nov 26, 2024

ClayBenson94 left a comment

Choose a reason for hiding this comment

adamodd commented Nov 26, 2024

adamodd commented Nov 26, 2024 • edited Loading

adamodd commented Nov 26, 2024

ClayBenson94 commented Nov 26, 2024

aterstriep commented Nov 27, 2024

aterstriep left a comment

Choose a reason for hiding this comment

aterstriep Nov 27, 2024

Choose a reason for hiding this comment

aterstriep Nov 27, 2024

Choose a reason for hiding this comment

adamodd Nov 27, 2024

Choose a reason for hiding this comment

adamodd Nov 27, 2024

Choose a reason for hiding this comment

aterstriep Dec 3, 2024

Choose a reason for hiding this comment

aterstriep Nov 27, 2024

Choose a reason for hiding this comment

adamodd commented Nov 27, 2024

adamodd commented Nov 27, 2024 • edited Loading

aterstriep commented Nov 27, 2024 • edited Loading

adamodd commented Nov 27, 2024

ClayBenson94 commented Dec 2, 2024

aterstriep commented Dec 3, 2024

adamodd commented Dec 3, 2024

aterstriep left a comment

Choose a reason for hiding this comment

adamodd commented Nov 22, 2024 •

edited

Loading

adamodd commented Nov 26, 2024 •

edited

Loading

adamodd commented Nov 27, 2024 •

edited

Loading

aterstriep commented Nov 27, 2024 •

edited

Loading