Skip to content

Conversation

shindigira
Copy link
Contributor

@shindigira shindigira commented Jan 12, 2024

Closes #177
Closes #190

Follow-up to https://github.com/cfpb/sbl-frontend/pull/146

Changes

  • Style(fix): Error Message Change - 'You must enter your first name to complete your user profile and access the system platform.'
  • Style(fix): Accessibility Change - Institution selection well - dotted line goes around the LEI, TIN and RSSD ID fields as well.
  • Style(fix): Removed interfering styles
  • Style(feat): Implements the new Checkbox (error/success/warning) and Checkbox Focus from DS/DSR
  • Feat(fix): 'Clear Form' no longer clears the list of associated financial institutions

Local Dev Testing

  • yarn up -i design-system-react to update DSR package with Checkbox status styling

Potential Follow-Up PRs

  • Margin change between the icon and text in Alerts
  • Vertically centering in Alerts

Screenshots

Focus: Red dotted border
Screenshot 2024-01-17 at 12 12 32 PM
Focus: dotted line around entire label
Screenshot 2024-01-17 at 12 00 36 PM

@shindigira shindigira changed the title wip 85 part 2 [Update] Complete Your User Profile - Field Inputs - Focus Styling(Error), Checkbox Group Focus Dotted Line Jan 17, 2024
Copy link

@natalia-fitzgerald natalia-fitzgerald left a comment

Choose a reason for hiding this comment

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

@shindigira - This is looking good! If possible I'd like to see it in live code before approving. Let me know if that's possible. Thanks!

@shindigira
Copy link
Contributor Author

@shindigira - This is looking good! If possible I'd like to see it in live code before approving. Let me know if that's possible. Thanks!

We are pushing up now and will be there in a few min.

@natalia-fitzgerald
Copy link

@shindigira
For the checkbox alignment can you confirm that what is live in the code (on AWS) pulls from the DS component? It looks like they match but I just wanted to confirm.

@shindigira shindigira requested a review from contolini as a code owner January 18, 2024 19:26
@shindigira shindigira removed the request for review from contolini January 18, 2024 19:32
@shindigira
Copy link
Contributor Author

@shindigira For the checkbox alignment can you confirm that what is live in the code (on AWS) pulls from the DS component? It looks like they match but I just wanted to confirm.

Confirmed that they match in both markup and css classes used:

@natalia-fitzgerald
Copy link

@shindigira
When I hover over the links in the form-level alert I'm seeing the cursor instead of the hand. I think it's because it's missing an href. Can we fix?
Screenshot 2024-01-18 at 9 15 29 PM

@shindigira
Copy link
Contributor Author

shindigira commented Jan 19, 2024

@shindigira When I hover over the links in the form-level alert I'm seeing the cursor instead of the hand. I think it's because it's missing an href. Can we fix? Screenshot 2024-01-18 at 9 15 29 PM

Thank you for this catch. And it definitely was the missing href attribute that ties to cursor:pointer.
I pushed the change to this PR.

@natalia-fitzgerald
Copy link

natalia-fitzgerald commented Jan 20, 2024

@shindigira
Can you double check the error content against the Figma file? It looks like there are a number of discrepancies - where the deployed page isn't matching the Figma file. I have not made recent changes to the content so I guess this was missed until now.

Inconsistency between Figma content and deployed page content

  • Form-level error links content
  • Field-level error alert - associate financial institution - content

Generally make sure that text content does not exceed 670px in width.

@shindigira
Copy link
Contributor Author

@shindigira Can you double check the error content against the Figma file? It looks like there are a number of discrepancies - where the deployed page isn't matching the Figma file. I have not made recent changes to the content so I guess this was missed until now.

Inconsistency between Figma content and deployed page content

  • Form-level error links content
  • Field-level error alert - associate financial institution - content

It was requested to change to what it currently is on AWS to maintain consistency between the error message (Field Level Alert) and the error link content.

I will revert back to match the Figma.

Generally make sure that text content does not exceed 670px in width.

Thanks for that! I made sure to add this around the Field Level Alerts in addition to the Paragraphs too.

@shindigira shindigira changed the title [Update] Complete Your User Profile - Field Inputs - Focus Styling(Error), Checkbox Group Focus Dotted Line [Update] Complete Your User Profile - Field Inputs - Focus Styling(Error/Warning/Success), Checkbox Group Focus Dotted Line Jan 23, 2024
@shindigira
Copy link
Contributor Author

Just waiting on the DS and DSR updates for the new Checkbox styling:

cfpb/design-system-react#285
cfpb/design-system#1883

@shindigira
Copy link
Contributor Author

@billhimmelsbach @meissadia Implemented the Updated DSR with Checkbox status stylings. Just need a dev review on this remaining part.

Then can push up to AWS for final @natalia-fitzgerald designer review

@shindigira shindigira changed the title [Update] Complete Your User Profile - Field Inputs - Focus Styling(Error/Warning/Success), Checkbox Group Focus Dotted Line [Update] Complete Your User Profile - Content text updates from Figma, Checkbox Group Status/Focus Style Update Jan 24, 2024
@natalia-fitzgerald
Copy link

@shindigira
I'm having trouble loading this page so that I can review. When I click on the "Sign in with Login.gov" button I am taken to the "personal email domain" notification page.

Once this gets worked out can you confirm which pages I should include in this review? Should it be the "Complete your user profile page, the errors, and the notification page?"

@shindigira
Copy link
Contributor Author

@shindigira I'm having trouble loading this page so that I can review. When I click on the "Sign in with Login.gov" button I am taken to the "personal email domain" notification page.

Once this gets worked out can you confirm which pages I should include in this review? Should it be the "Complete your user profile page, the errors, and the notification page?"

Unless there is a new error (due to backend changes), this requires associating your email domain with an institution on the backend. Will need @billhimmelsbach to reset this as I have yet to be acclimated to accessing the AWS deployment.

@billhimmelsbach
Copy link
Contributor

@natalia-fitzgerald @shindigira

Don't know if you saw it on the channel, but the latest of this branch is on AWS now as of 12:30 PT / 3:30 ET. Thanks! 👍

Copy link

@natalia-fitzgerald natalia-fitzgerald left a comment

Choose a reason for hiding this comment

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

@shindigira - This is looking great. I have one small adjustment and then it should be good to go.

  • Reduce the space between the bottom of the text introduction and the notification to 45px?

Copy link
Contributor

@billhimmelsbach billhimmelsbach left a comment

Choose a reason for hiding this comment

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

I think there's a "Clear form" bug that we'll want to fix, where clicking it removes all financial institution choices.

clear-form-bug.mov

@shindigira
Copy link
Contributor Author

shindigira commented Jan 26, 2024

fix: Clear Form no longer unmounts Associated Financial Data

@meissadia @billhimmelsbach Thanks. The fix is in this commit. When testing, please follow the steps:

  1. Associate the user's email domain with more than one financial institution (dbreaver or api).
  2. Navigate to http://localhost:8899/profile-form
  3. Click to check the checkboxes of the institutions
  4. Click 'Clear Form'
  5. Click to check the checkboxes of the institutions again
  6. Hit 'Submit' and check that the request includes the checked institutions
  7. Refresh the app and verify the associated institutions are at: http://localhost:8899/user-profile

Copy link
Contributor

@billhimmelsbach billhimmelsbach left a comment

Choose a reason for hiding this comment

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

Looks good! Natalia is going to take a peek at it, then it should be good to go. 👍

Copy link

@natalia-fitzgerald natalia-fitzgerald left a comment

Choose a reason for hiding this comment

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

Great work @shindigira!

@shindigira shindigira merged commit e667aa9 into main Jan 26, 2024
@billhimmelsbach billhimmelsbach deleted the 85-part-2 branch May 31, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants