-
Notifications
You must be signed in to change notification settings - Fork 1
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
DREF Import template #828
base: develop
Are you sure you want to change the base?
DREF Import template #828
Conversation
🦋 Changeset detectedLatest commit: bd703a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
7125663
to
ddf26ff
Compare
ddf26ff
to
f9dc2f6
Compare
f9dc2f6
to
97395de
Compare
97395de
to
3c78067
Compare
3c78067
to
1bd554a
Compare
1bd554a
to
69877b7
Compare
69877b7
to
c60041b
Compare
c60041b
to
876a1bb
Compare
876a1bb
to
3a7c3ae
Compare
const name = row.getCell(1)?.name; | ||
const value = getValueFromCellValue(row.getCell(2)?.value); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add NOTE why we are using cell 1 and cell 2.
"drefImportFailedDescription": "Failed to process the selected import file, make sure you've downloaded the import template from the GO platform.", | ||
"drefImportButtonDescription": "Failed to process the selected import file, make sure you've downloaded the import template from the GO platform." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need two different strings with the same content?
export interface Props { | ||
drefId: number; | ||
id: number; | ||
status: DrefStatus | null | undefined; | ||
status: null | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason that we have status that can be only null
or undefined
?
|
||
export const headerRowStyle: Partial<Style> = { | ||
font: { | ||
name: 'Montserrat', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have a constant for the font name?
const firstRow = row.getCell(col); | ||
firstRow.style = { | ||
...firstRow.style, | ||
fill: { | ||
...firstRow.style?.fill, | ||
...alternateRowFill, | ||
}, | ||
}; | ||
const secondRow = row.getCell(col + 1); | ||
secondRow.style = { | ||
...secondRow.style, | ||
fill: { | ||
...secondRow.style?.fill, | ||
...alternateRowFill, | ||
}, | ||
}; | ||
const thirdRow = row.getCell(col + 2); | ||
thirdRow.style = { | ||
...thirdRow.style, | ||
fill: { | ||
...thirdRow.style?.fill, | ||
...alternateRowFill, | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be renamed to firstCell, secondCell, thirdCell
SHEET_OPERATION_OVERVIEW, | ||
{ properties: { tabColor: { argb: hexToArgb(COLOR_PRIMARY_RED, '10') } } }, | ||
); | ||
// TODO: Add red color to all the sheet tabs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has been fixed. We can remove the comment.
|
||
options.forEach((option: TypeOfDrefEnum, i: number) => { | ||
const cell = optionsWorksheet.getCell(i + 2, column.number); | ||
cell.name = `${key}____${option.key}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should standardize this operation with a function or a constant.
listToGroupList( | ||
templateActions, | ||
(templateAction) => { | ||
const fieldName = String(templateAction.name).split('__')[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should standardize this operation with a function or a constant.
worksheet.mergeCells(i + ROW_OFFSET, 1, i + ROW_OFFSET, 3); | ||
lastHeadingIndex = i + 1; | ||
} else if (templateAction.type === 'input') { | ||
const mode = (i - lastHeadingIndex) % 2 === 0 ? 'listHeading' : 'heading'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a NOTE on how this logic works.
<RadioInput | ||
name={undefined} | ||
label="Select type of DREF for template" | ||
options={dref_dref_dref_type} | ||
keySelector={typeOfDrefKeySelector} | ||
labelSelector={stringValueSelector} | ||
value={typeOfDref} | ||
onChange={setTypeOfDref} | ||
// Only response type is available for now | ||
disabled | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should disable everything except the "Response" type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a requirement from client.
645f90a
to
773389a
Compare
773389a
to
17b2870
Compare
17b2870
to
2933184
Compare
2933184
to
49d8473
Compare
49d8473
to
5e71084
Compare
5e71084
to
bd703a2
Compare
TODO