Skip to content

PM-1504 edit create scorecard#1196

Merged
vas3a merged 10 commits intofeat/reviewfrom
PM-1504_edit-create-scorecard
Aug 19, 2025
Merged

PM-1504 edit create scorecard#1196
vas3a merged 10 commits intofeat/reviewfrom
PM-1504_edit-create-scorecard

Conversation

@vas3a
Copy link
Collaborator

@vas3a vas3a commented Aug 19, 2025

Related JIRA Ticket:

https://topcoder.atlassian.net/browse/PM-1504 - Create/Edit Scorecard UI

What's in this PR?

Added page for edit/create scorecard
image

image image


interface ScorecardResponse {
scorecard: Scorecard | undefined
error?: any

Choose a reason for hiding this comment

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

Consider specifying a more precise type for error instead of using any to improve type safety and clarity.

}

export type EditScorecardPageContextValue = {
confirm: (prosp: ConfirmationProps) => Promise<boolean>,

Choose a reason for hiding this comment

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

Typo in parameter name: prosp should be props.

}, [params.scorecardId, navigate])

if (scorecardQuery.isValidating) {
return <></>

Choose a reason for hiding this comment

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

Returning an empty fragment when scorecardQuery.isValidating is true might not provide the best user experience. Consider displaying a loading indicator to inform users that data is being fetched.

}): ReactElement {
// Expecting a single React element as children (the input)
if (!props.children || Array.isArray(props.children)) {
return <></>

Choose a reason for hiding this comment

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

The check for Array.isArray(props.children) might not be sufficient to ensure that props.children is a single React element. Consider using React.Children.only to enforce that props.children is a single child.

name: yup.string()
.required('Group name is required'),
weight: yup
.number()

Choose a reason for hiding this comment

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

Consider using .positive('Weight must be a positive number') instead of .min(0, 'Weight must be at least 0') for clarity and consistency in validation messages.

if (!items?.length) return false
const sum = items.reduce((acc, g) => acc + (Number(g.weight) || 0), 0)

if (sum !== 100) {

Choose a reason for hiding this comment

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

The condition if (sum !== 100) should use the value parameter instead of hardcoding 100. Change it to if (sum !== value) to ensure the function behaves correctly with different values.

readonly hideInlineErrors?: boolean
readonly hint?: string
readonly label: string | JSX.Element
readonly label?: string | JSX.Element

Choose a reason for hiding this comment

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

The label prop has been changed from required to optional. Verify that the component can handle cases where label is undefined, as this might impact accessibility or user experience.

`${baseUrl}/scorecards/${id}`,
const { data, error, isValidating }: SWRResponse<Scorecard, any> = useSWR<Scorecard>(
// eslint-disable-next-line unicorn/no-null
id ? `${baseUrl}/scorecards/${id}` : null,

Choose a reason for hiding this comment

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

The URL path has been changed from \${baseUrl}/${id}`to`${baseUrl}/scorecards/${id}``. Ensure that this change is intentional and that the backend API endpoint supports this new path. If the endpoint has changed, verify that all related documentation and tests are updated accordingly.

@vas3a vas3a merged commit 136bb49 into feat/review Aug 19, 2025
3 checks passed
@vas3a vas3a deleted the PM-1504_edit-create-scorecard branch August 19, 2025 16:35
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.

2 participants