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

[Design Review] [Resolve Errors] [Review Warnings] #477

Merged
merged 31 commits into from
May 15, 2024

Conversation

shindigira
Copy link
Contributor

@shindigira shindigira commented May 9, 2024

closes #485
closes #488
closes #481
closes #482

Note

  • yarn install to get the DSR updates

Changes

  • chore(filingTypes): added total_count to a filing type
  • fix(record_no): In the Error/Warnings tables, incremented the row number by one
  • style(pagination/tables): extra unused rows are now white
  • style(Resolve Errors/Warnings): underlined error links
  • content(Resolve Errors): Content updated to match the figma
  • content(Review Warnings): Content updated to match the figma
  • deps(DSR): Updated DSR's pagination component - button content should be centered

Todo

  • tables - word break and handling large character sets
  • filing nav buttons - refactor @meissadia
  • react-hook-form error handling the verify warnings section @meissadia

@shindigira shindigira marked this pull request as draft May 9, 2024 00:19
@shindigira shindigira changed the title wip: Resolve errors bugs 1 wip: Resolve errors May 13, 2024
@shindigira shindigira changed the title wip: Resolve errors wip: [Resolve Errors] [Review Warnings] May 13, 2024
@shindigira shindigira changed the title wip: [Resolve Errors] [Review Warnings] [Design Review] [Resolve Errors] [Review Warnings] May 13, 2024
@shindigira shindigira marked this pull request as ready for review May 13, 2024 23:02
src/index.css Outdated
Comment on lines 56 to 59
td {
background-color: transparent !important;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Odd that this is needed but I'm not able to spot why the striped background is appearing.

I think we should consider making this a more tightly scoped class to avoid blocking future devs from being able to add striped tables if needed.

Something like table.no-stripes > td

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure! I'll leave this in and put a TODO note.
cfpb/design-system-react#345

Copy link
Contributor

Choose a reason for hiding this comment

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

Heyo! Let's label these @shindigira with a post-mvp label even in the design-system-react repo, just so we can grab all of them easily later. 👍

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! I'll let @meissadia take another peek and give the final approval!

meissadia
meissadia previously approved these changes May 15, 2024
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 shindigira merged commit fc791a7 into main May 15, 2024
4 checks passed
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