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

Design Review - Errors & Warnings - Part 2 #537

Merged
merged 25 commits into from
May 25, 2024
Merged

Design Review - Errors & Warnings - Part 2 #537

merged 25 commits into from
May 25, 2024

Conversation

shindigira
Copy link
Contributor

@shindigira shindigira commented May 17, 2024 •
edited

closes #538
closes #539
closes #542
closes #551

More changes (05/21/2024)

  • content(errors/warnings): error/warnings counts and table rows use commas (e.g. 1,000)
  • stylefix(table): hidden table borders are now present again on single/register fields
  • content(errors/warnings): record/records changed to 'found'

More changes (05/20/2024)

  • style(success error/warnings alert): reduced margin-bottom to 30px (30px between success alert and Filing Nav Buttons)
  • style(warnings): verify text limited to 670px, well limited to 770px
  • style(warnings): multifield warnings now also has table borders
  • feat(errors & warnings): Updated the count to be the rows affected by errors/warnings
  • content(errors/warnings): added 'record' or 'records' word to count statement

Changes

  • style(single/multi/register errors): 45px margin-bottom
  • style(tables): only multifield errors tables have borders,
  • content(errors): removed bottom alerts
  • style(errors/warnings - success): use negative margin (ensures 30px from alert to filing nav buttons)
  • content(errors): "there is" instead of "there may be"
  • style(warnings): restrict header to 670px
  • enhancement(warnings): Reworked warnings alert logic

How to test (dev)

The only functionality changes are in:

  • enhancement(warnings): Reworked warnings alert logic -- see FilingWarningsAlerts.tsx
  • feat(errors & warnings): Updated the count to be the rows affected by errors/warnings -- see getRecordsAffected function.

Note

@shindigira shindigira marked this pull request as draft May 17, 2024 17:16
@shindigira shindigira changed the title WIP: Design Review - Errors WIP: Design Review - Errors & Warnings - Part 2 May 17, 2024
@shindigira shindigira changed the title WIP: Design Review - Errors & Warnings - Part 2 Design Review - Errors & Warnings - Part 2 May 17, 2024
@shindigira shindigira marked this pull request as ready for review May 17, 2024 21:39
@shindigira shindigira marked this pull request as draft May 18, 2024 09:21
@shindigira shindigira marked this pull request as ready for review May 20, 2024 15:25
@shindigira
feat(errors & warnings): Updated the count to be the rows affected by…
7cbfb69 
… errors/warnings
@shindigira
style(success error/warnings alert): reduced margin-bottom to 30px
ae2d4b9
@shindigira
style(warnings): multifield warnings now also has table borders
ec0e028
@shindigira
style(verify warnings): verify text limited to 670px
aa31b68
@shindigira
comment - useisoverflowing
4674173
@shindigira
content(errors/warnings): added 'records' word to count
6904f2f
@shindigira
style(verify warnings): 670px checkbox text; 770px well
b6ef2f9
@shindigira
content(errors/warnings): records vs record
19d24e6
@shindigira
stylefix(table): hidden table borders are now present again
ed8b0a1
@shindigira
content(errors/warnings): error/warnings counts use commas (e.g. 1,000)
f641232
@shindigira
Merge branch 'main' of github.com:cfpb/sbl-frontend into design-revie…
3078afd 
…w-errors
@shindigira
content(errors/warnings): records changed to found
0872c3d
@shindigira
stylefix(upload/errors): removed min-h-[80vh]
bf7daf5
meissadia
meissadia requested changes May 24, 2024
View reviewed changes
Copy link
Collaborator

@meissadia meissadia left a comment •
edited

Choose a reason for hiding this comment

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

Code looks good, awesome work navigating the complexities of these components!

There does seem to be some inconsistency with table borders. Based on the design file I see three scenarios:

  • Paginated table => has border on all sides
  • Horizontally scrollable table => Has border on top + left + bottom
  • Fully displayed table => Only has bottom border

I'm currently seeing some "fully displayed tables" with all borders, which seems incorrect. (sorry, I can't seem to find which test file this was generated with).
border-variation

I also see some paginated tables with only the bottom border, when I would expect all borders.

  • Test file: logic-errors_medium.csv
    Screenshot 2024-05-24 at 10 18 14 AM

@shindigira
Copy link
Contributor Author

shindigira commented May 25, 2024 •
edited

@meissadia Thanks for the thorough review and noticing the tables border styling wonkiness.

Currently, what you see is by intent; the style requirements for table borders from @natalia-fitzgerald are as:

  1. Singlefield/Register-level errors and warnings don't have the full borders styling; the bottom border comes from the last table row <tr /> element of the table.
  2. Multifield errors and warnings have the full borders styling.
  3. The "hidden table" on the last page of paginated tables has the full borders styling.

In a later PR, we will address this specific styling issue: #547 (comment)

@shindigira shindigira requested a review from meissadia May 25, 2024 03:20
meissadia
meissadia approved these changes May 25, 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.

@shindigira Thanks for the clarification.

@shindigira shindigira merged commit cb1e7cb into main May 25, 2024
4 checks passed
@billhimmelsbach billhimmelsbach deleted the design-review-errors branch May 31, 2024 14:44
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 Awaiting requested review from billhimmelsbach billhimmelsbach is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@shindigira @meissadia