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.

Sign up for GitHub

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

Jump to bottom

[Page] Resolve Errors & Warnings #423

Merged
merged 118 commits into from May 6, 2024
Merged

[Page] Resolve Errors & Warnings #423

merged 118 commits into from May 6, 2024

Conversation

shindigira
Copy link
Contributor

@shindigira shindigira commented Apr 26, 2024 •
edited

closes #333
closes #401

Note (05/02/24)

The sbl-filing branch used is main -- latest working commit 0ddf51bd4bd0ab7aefac8cbb02c7797077885620

branched off 331-filing-routing_phase2-live-data

Changes

  • feat(Resolve Errors): Single-Field Errors
  • feat(Resolve Errors): Multi-Field Errors
  • feat(Resolve Errors): Register-Level Errors
  • feat(Resolve Errors): Alerts present depending on errors or no errors
  • feat(Resolve Errors): Swappable between Step 1 (Syntax Errors) and Step 2 (Logical Errors/Register Errors); navigates to warnings on Step 2 (no errors)
  • enhancement(Upload): added an enableLongPolling boolean to useGetSubmissionLatest; default false
  • enhancement(Resolve Errors): Implemented 'react-markdown' -- Validation Description
  • chore(Resolve Errors): TypeScript handling in FilingErrors.helpers.ts
  • chore(Upload): Hide FilingNavButtons if still loading
  • feat(Resolve Errors): if a get submission latest fails, there is an Alert that pops up

Next PRs

  • Pagination
  • Accessibility pass
  • Table styling size adjustments
  • Download linking the validation report

How to Test

  1. yarn install to install react-markdown
  2. Select an institution and upload with the csv files provided (Syntax Errors and then Logical/Register Errors)
  3. Ensure the right Alert occurs based on the error (or no errors) per segment
  4. Verify all proper data is present per field-level section

Test CSV's to use

all-errors-warnings-no-syntax-errors.csv
sblar_syntax_errors_large.csv
sblar_syntax_errors.csv

Note: If Syntax Errors are present, then validation for the logical errors/warnings do not occur. Hence, why the test CSV's has either syntax errors or logical errors; not both.

Screenshots

Markdown
Screenshot 2024-05-02 at 2 40 05 AM
SingleFieldErrors Success Register-Level
Screenshot 2024-05-01 at 4 44 52 PM Screenshot 2024-05-01 at 4 45 10 PM Screenshot 2024-05-01 at 4 45 45 PM

@meissadia
fix: [axiosService] Add missing POST parameter
8688b34
@meissadia
[Filing Routing] Phase 1 - Mock Filing progress
6822d26
@meissadia
[Filing Routing] Phase 1 - Separate routes per step
9849158
@meissadia
fix: axiosService.request - remove unneeded data param in favor of …
fea91e3 
…`body`
@meissadia
fix: useMemo to avoid duplicate placeholder objects being generated
46971d5
@meissadia
fix: FilingStepWrapper.helpers - remove unused eslint rule disables
2c2de5f
@meissadia
[Filing Routing] Phase 1 - Mock Filing progress
bc14e84
@meissadia
[Filing Routing] Phase 1 - Separate routes per step
2cbd121
@meissadia
fix: useMemo to avoid duplicate placeholder objects being generated
e7d28d1
@meissadia
fix: FilingStepWrapper.helpers - remove unused eslint rule disables
9c52113
@meissadia
Update imports after rebase
d912fc7
@meissadia
Update imports after rebase part 2
9257eb7
@meissadia
Add "tasks" to our mock filing
c60e6e6
@meissadia
Sync axiosService file
2867e7b
@meissadia
Filing step - Display "current" highlight even if step is incomplete
cd9a385
@meissadia
Filing - Placeholder for "is step complete?" status
b937bc4
@meissadia
Filing step - Improve spacing between step indicators
5b6c4f3
@meissadia
Filing steps - Remove custom hook that contained routes, which helps …
c0be487 
…flatten the routing structure and hopefully simplify maintenance
@meissadia
Merge branch 'main' into 331-filing-routing_phase1-mock-data
f66f7b3
@meissadia
Filing steps - Revamp flex alignment of page elements
0c0d989
@meissadia
[ScreenReaderOnly] Component: Extract from Filing step indicator code…
cd69c7b 
… for reusability.
@meissadia
Filing steps - More dynamic routes (year and lei from the url)
e64c41c
@meissadia
Filing steps - Add TODO to check on StepIndicator widths
a0dcab9
@meissadia
Merge branch 'main' into 331-filing-routing_phase1-mock-data
74506f3
@meissadia
fix: [axiosService] Always include "body" for post/put/patch requests
f8f1bdc
@meissadia
feat: [API]
502c82c 
- Filing creation
- Rename fetchFilingSubmissionLatest to fetchSubmissionLatest
- Created useFilingAndSubmissionInfo
@meissadia
feat: [FilingOverview] Determine and link to next incomplete step
152ceb6
@meissadia
feat: [Filing] Display current Institution's progress within the subm…
4ebc759 
…ission process
@meissadia
feat: [Filing] Placeholder pages for Filing steps
a0fa68b
@meissadia
fix: [Institution card] Routing logic for a Submissio n with a previo…
77c7ea0 
…us upload
@shindigira shindigira changed the title Resolve Errors - Phase 1 Resolve Errors & Warnings - Phase 1 May 3, 2024
@billhimmelsbach billhimmelsbach mentioned this pull request May 3, 2024
Merged
@shindigira shindigira changed the title Resolve Errors & Warnings - Phase 1 Resolve Errors & Warnings May 3, 2024
@shindigira shindigira changed the title Resolve Errors & Warnings [Page] Resolve Errors & Warnings May 3, 2024
meissadia
meissadia requested changes May 6, 2024
View reviewed changes
Copy link
Collaborator

@meissadia meissadia left a comment

Choose a reason for hiding this comment

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

Couple cleanup points, but the major find is the Subheading content for Errors step 2. Looking great though!

src/pages/Filing/FilingApp/FieldEntry.tsx Show resolved Hide resolved
src/pages/Filing/FilingApp/FilingErrors/index.tsx Outdated Show resolved Hide resolved
@shindigira
Merge branch 'design-review-upload' of github.com:cfpb/sbl-frontend i…
0254a14 
…nto 333-resolve-errors-warnings-skeleton-phase1
@shindigira shindigira mentioned this pull request May 6, 2024
Open
@shindigira shindigira requested a review from meissadia May 6, 2024 19:00
@shindigira shindigira requested a review from meissadia May 6, 2024 19:58
meissadia
meissadia approved these changes May 6, 2024
View reviewed changes
Copy link
Collaborator

@meissadia meissadia left a comment

Choose a reason for hiding this comment

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

👍🏾

billhimmelsbach
billhimmelsbach approved these changes May 6, 2024
View reviewed changes
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.

This is super great! (good eye @meissadia finding the content changes)

Looks like we're missing blue underlines under some links that are in the Figma, could you create a separate ticket or fix that here?
Screenshot 2024-05-06 at 3 55 14 PM
Screenshot 2024-05-06 at 3 55 25 PM

@shindigira
Copy link
Contributor Author

shindigira commented May 6, 2024 •
edited

This is super great! (good eye @meissadia finding the content changes)

Looks like we're missing blue underlines under some links that are in the Figma, could you create a separate ticket or fix that here? Screenshot 2024-05-06 at 3 55 14 PM Screenshot 2024-05-06 at 3 55 25 PM

Thanks for the find. This appears to be a DS requirement for links to be nested under a p, li or dd element.

We should address this outside of this PR since this styling change could affect all links through the app.
Screenshot 2024-05-06 at 4 28 07 PM

@billhimmelsbach
Copy link
Contributor

We should address this outside of this PR since this styling change could affect all links through the app.

Sounds good, can we make a ticket for it, then this is good to go @shindigira!

@shindigira shindigira mentioned this pull request May 6, 2024
Open
@shindigira
Copy link
Contributor Author

We should address this outside of this PR since this styling change could affect all links through the app.

Sounds good, can we make a ticket for it, then this is good to go @shindigira!

#456

@shindigira shindigira merged commit 4429fa1 into main May 6, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meissadia meissadia meissadia approved these changes

@billhimmelsbach billhimmelsbach billhimmelsbach approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Page] Multifield Errors [Page][Resolve Errors/Warnings] Basic skeleton of errors and warnings
3 participants
@shindigira @billhimmelsbach @meissadia