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.

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

Help Improve Refactor #165

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open

Conversation

collinschreyer-dev
Copy link
Collaborator

Feedback Form Updates

image

Overview

Updates feedback form component to align with UI team recommendations.

Key Changes

User Interface

  • Simplified form layout and structure
  • Enhanced status badge states (Compliant, Non-compliant, Cannot Evaluate)
  • Improved form element styling (radio buttons, checkboxes)

Technical Updates

  • Improved survey handling with duplicate removal and better sorting
  • Updated alert messaging system

Testing Completed ✓

  • Verified all status badge states and colors
  • Tested complete submission flow including resubmission
  • Validated accessibility compliance including screen reader compatibility

Ready for review.

Copy link
Collaborator

@BuckinghamAJ BuckinghamAJ left a comment

Choose a reason for hiding this comment

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

Looking good, but wanted to get your thoughts on a few comments I left.

Great job!

<main class="main-content">
<div class="srt-title-padding">
<a [routerLink]="['/solicitation/report']" class="back-link">
< Back to Solicitation Review Results for Section 508 Requirements
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking we could add a Google Analytics function to track this click if we think it is necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea!

after my next commit, we'll be able to track:

  • Back navigation click - navbar_solicitation_report
  • Solicitation number link clicks - solicitation_link
  • Feedback form submissions - feedback_submit

this.surveyModel.push('');
}
// Remove duplicates based on ID
this.surveys = [...new Map(data.map(item => [item.ID, item])).values()]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at this, why don't we just have restriction in the database not allowing the same ID to be submitted? Seems like more work on the API server when we could just make sure we have that db constraint in place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes perfect sense! I'll plan to do the following:

  1. Add the unique constraint to the surveys table
  2. Remove the duplicate filtering from the frontend

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Duplicate Surveys Issue Resolved

I compared my local database with the production database and discovered that the production environment contained only the correct three survey entries. My local database, however, had six entries, duplicating the initial three. This discrepancy explains the duplicate display we were observing. Since we currently maintain a fixed set of surveys and don't foresee adding new ones in the immediate future, we can simplify the solution by addressing the data directly and removing redundant frontend logic.

Actions Taken:

  • Corrected the Surveys table in the database to match production (3 unique rows).
  • Removed unnecessary duplicate filtering from the frontend Angular code
  • Updated getSurveys function (sorting by ID added).

Unique Constraint on hold:

Given the fixed set of surveys, I've decided to postpone adding a unique constraint to the id column. If our requirements change and we need to add surveys dynamically, I can revisit adding the constraint then.

Thanks for suggesting the database constraint approach! It helped pinpoint the root cause.

Copy link
Collaborator

@BuckinghamAJ BuckinghamAJ left a comment

Choose a reason for hiding this comment

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

Everything looks good, just want you to look into 1 thing I mentioned in the review.

Once that is resolved we should be good to move forward.

try {
const navigation = this.router.getCurrentNavigation();
if (navigation?.extras?.state?.['solicitation']) {
const data = navigation.extras.state['solicitation'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will we run into issues with trying to change the values inside the "const"? I thought "const" are not able to be reassigned but not sure how that affects the underlining values.

Additional, did you confirm that the changes made in one function carries onto the next function because sometimes if the variable scoping is off it would result in no underlining changes actually making its way through each function call.

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.

2 participants