Skip to content

[APP-707] feat: add save report modal#3364

Open
emyl3 wants to merge 9 commits into
mainfrom
el/app-707/save-ui
Open

[APP-707] feat: add save report modal#3364
emyl3 wants to merge 9 commits into
mainfrom
el/app-707/save-ui

Conversation

@emyl3

@emyl3 emyl3 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Description

  • Add the Save Report Modal
  • Create a report where the ownerUID is the same as the user you are logging in with to test and see the 'Save' button

Recording

savereport.page.mov

Tickets

Checklist before requesting a review

  • PR focuses on a single story
  • Code has been fully tested to meet acceptance criteria
  • PR is reasonably small and reviewable (Generally less than 10 files and 500 changed lines)
  • All new functions/classes/components reasonably small
  • Functions/classes/components focused on one responsibility
  • Code easy to understand and modify (clarity over concise/clever)
  • PRs containing TypeScript follow the Do's and Don'ts
  • PR does not contain hardcoded values (Uses constants)
  • All code is covered by unit or feature tests

@emyl3 emyl3 force-pushed the el/app-707/save-ui branch from ba2513e to 44c28e5 Compare June 24, 2026 14:49
@emyl3 emyl3 changed the title feat: add save report modal [APP-707] feat: add save report modal Jun 24, 2026
@emyl3 emyl3 marked this pull request as ready for review June 24, 2026 14:58
@emyl3 emyl3 requested a review from a team as a code owner June 24, 2026 14:58
@emyl3 emyl3 requested review from JordanGuinn and brick-green and removed request for a team June 24, 2026 14:58

@brick-green brick-green left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Couple little things

Comment on lines +181 to +183
.catch((err) => {
setError(JSON.stringify(err));
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

q, maybe b: what does the shape of this err look like? Is there a message or body or something to be used?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if it ever made it's way into the UI given it's handled API-side through exception handlers, but 4xx/5xx errors from all reports endpoints use the ErrorResponseBody interface for their response bodies, which look like:

{
   message: String;
}

If we want to use that here!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OOO Thank you!! I'll update!

);

const redirectToManageReport = () => {
window.location.href = '/nbs/nfc?ObjectType=7&OperationType=116';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gemini recommended something like this:

Suggested change
window.location.href = '/nbs/nfc?ObjectType=7&OperationType=116';
const navigate = useNavigate();
navigate('/nbs/nfc?ObjectType=7&OperationType=116');

as I understand it window.location.href forces a full page reload which perhaps isn't always necessary. A little out of my depths here though. Thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 here, I think we lose all application state on a forced page reload like this - would def prefer navigation through the React router if we're able!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think we are able to since that part of the app hasn't been modernized 😢 Which is why I believe we are doing this similarly here

@emyl3 emyl3 requested a review from a team as a code owner June 25, 2026 19:05
@emyl3 emyl3 requested review from timoballard and removed request for a team June 25, 2026 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants