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

refactor: naics UI improvements #1565

Merged
merged 4 commits into from
Mar 29, 2021
Merged

Conversation

dleard
Copy link
Contributor

@dleard dleard commented Mar 25, 2021

  • More descriptive language in the delete confirmation dialog
  • Two action buttons for clarity: "Delete" (red) and "Cancel" ('secondary' dark grey)
  • Acronyms like NAICS should be capitalized ("New Naics Code" button -> "New NAICS Code")
  • When a new code is added, on re-opening the New dialog again to add a subsequent one, the form validation styles should be cleared (not showing red/green).
  • Warning & block mutation when adding a NAICS code that already exists AND is currently active (follow the pattern: delete -> re-add)

@dleard dleard requested a review from kriscooke March 25, 2021 20:10
Comment on lines 29 to 35
let codeExists = false;
query.allNaicsCodes.edges.forEach((edge) => {
if (edge.node.naicsCode === naicsCode) {
codeExists = true;
}
});
return codeExists;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use some(...), it returns on the first found occurrence instead of looping through the entire array

Suggested change
let codeExists = false;
query.allNaicsCodes.edges.forEach((edge) => {
if (edge.node.naicsCode === naicsCode) {
codeExists = true;
}
});
return codeExists;
return query.allNaicsCodes.edges.some((edge) =>
edge.node.naicsCode === naicsCode
);

Copy link
Contributor

@kriscooke kriscooke left a comment

Choose a reason for hiding this comment

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

It's looking great! However while the below is no longer an issue, the red/green validation doesn't seem to be happening at all anymore 🤔

When a new code is added, on re-opening the New dialog again to add a subsequent one, the form validation styles should be cleared (not showing red/green).

@dleard dleard force-pushed the refactor/naics-ui-improvements branch from 5bef8e0 to efd556d Compare March 29, 2021 16:12
@dleard dleard force-pushed the refactor/naics-ui-improvements branch from efd556d to 111fe11 Compare March 29, 2021 17:41
@dleard dleard requested review from kriscooke and pbastia March 29, 2021 18:18
@dleard dleard merged commit aff729f into develop Mar 29, 2021
@dleard dleard deleted the refactor/naics-ui-improvements branch March 29, 2021 19:54
@bc-cas-shipit bc-cas-shipit bot temporarily deployed to dev March 29, 2021 20:25 Inactive
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.

3 participants