-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat: merge select and edit tabs in config #14137
base: main
Are you sure you want to change the base?
Conversation
…while developing StudioCodeListEditor behind a featureFlag.
# Conflicts: # frontend/packages/ux-editor/src/components/config/editModal/EditOptions/EditOptions.test.tsx # frontend/packages/ux-editor/src/components/config/editModal/EditOptions/OptionTabs/EditManualOptionsWithEditor/EditManualOptionsWithEditor.tsx # frontend/packages/ux-editor/src/components/config/editModal/EditOptions/OptionTabs/OptionTabs.tsx # frontend/packages/ux-editor/src/components/config/editModal/EditOptions/OptionTabs/hooks/useOptionListEditorTexts.ts
# Conflicts: # frontend/packages/ux-editor/src/components/config/editModal/EditOptions/OptionTabs/EditOptionList-v1/EditOptionList.tsx
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14137 +/- ##
==========================================
+ Coverage 95.32% 95.34% +0.01%
==========================================
Files 1780 1786 +6
Lines 23159 23292 +133
Branches 2689 2711 +22
==========================================
+ Hits 22077 22208 +131
- Misses 835 836 +1
- Partials 247 248 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
@@ -1525,6 +1525,10 @@ | |||
"ux_editor.modal_header_type_helper": "Velg titteltype", | |||
"ux_editor.modal_new_option": "Legg til alternativ", | |||
"ux_editor.modal_properties_add_radio_button_options": "Hvordan vil du legge til radioknapper?", | |||
"ux_editor.modal_properties_code_list": "Velg fra biblioteket", | |||
"ux_editor.modal_properties_code_list_alert_title": "Du er i ferd med å redigere en kodeliste i biblioteket.", |
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.
"ux_editor.modal_properties_code_list_alert_title": "Du er i ferd med å redigere en kodeliste i biblioteket.", | |
"ux_editor.modal_properties_code_list_alert_title": "Du redigerer nå en kodeliste i biblioteket.", |
"ux_editor.options.section_heading": "Valg for kodelister", | ||
"ux_editor.options.single": "{{value}} alternativ", | ||
"ux_editor.options.tab_code_list": "Velg kodeliste", | ||
"ux_editor.options.tab_manual": "Sett opp egne alternativer", | ||
"ux_editor.options.tab_referenceId": "Angi referanse-ID", | ||
"ux_editor.options.upload_title": "Last opp din egen", |
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.
"ux_editor.options.upload_title": "Last opp din egen", | |
"ux_editor.options.upload_title": "Last opp egen kodeliste", |
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.
Ser bra ut dette @Konrad-Simso og @ErlingHauan. Har justert litt på et par tekster, men ingen krise om dere er uenige og avviser dem :-D.
...ents/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/config/editModal/EditOptions/EditOptions.test.tsx
Show resolved
Hide resolved
queryClientMock.clear(); | ||
}); | ||
afterEach(() => queryClientMock.clear()); | ||
|
||
it('should render', async () => { |
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('should render', async () => { | |
it('should render', () => { |
}, | ||
}); | ||
expect(await screen.findByText(textMock('ux_editor.modal_new_option'))).toBeInTheDocument(); | ||
it('should render spinner when loading data', async () => { |
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('should render spinner when loading data', async () => { | |
it('should render spinner when loading data', () => { |
.modalTrigger { | ||
width: 50%; | ||
} | ||
|
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 see where you are going with this, but when the column width is made small, the library button becomes a little squashed. I suggest adding min-width: 12rem
to the modalTrigger
classes, so that the button labels remain on one line.
Before:
before.mp4
After:
after.mp4
...ponents/config/editModal/EditOptions/OptionTabs/EditOptionChoice/EditOptionChoice.module.css
Show resolved
Hide resolved
).toBeInTheDocument(); | ||
}); | ||
|
||
it('should editOptionsId to blank when removing choice', async () => { |
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('should editOptionsId to blank when removing choice', async () => { | |
it('should change editOptionsId to blank when removing choice', async () => { |
const isOptionChosen = | ||
(component.optionsId !== undefined && component.optionsId !== '') || | ||
component.options !== 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.
I think this part could be more clear 🤔
The logic could be wrapped inside variables, and the variable name isOptionChosen
can be made more explicit. Maybe something like componentHasOptionList
:
const isOptionChosen = | |
(component.optionsId !== undefined && component.optionsId !== '') || | |
component.options !== undefined; | |
const hasOptionsId = component.optionsId !== undefined && component.optionsId !== ''; | |
const componentHasOptionList = hasOptionsId || component.options; |
Or maybe we can even just say:
const isOptionChosen = | |
(component.optionsId !== undefined && component.optionsId !== '') || | |
component.options !== undefined; | |
const componentHasOptionList = component.optionsId || component.options; |
const shouldDisplayChosenOption = isOptionChosen && chosenOption === true; | ||
|
||
return ( | ||
<> | ||
{shouldDisplayChosenOption ? ( |
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.
Would it be possible to simplify this, by replacing shouldDisplayChosenOption
with chosenOption
?
const shouldDisplayChosenOption = isOptionChosen && chosenOption === true; | |
return ( | |
<> | |
{shouldDisplayChosenOption ? ( | |
return ( | |
<> | |
{chosenOption ? ( |
@@ -0,0 +1,3 @@ | |||
.modalTrigger { | |||
width: 50%; |
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.
See the other comment regarding the modalTrigger
class 😊
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.
In this file, waitFor
can be removed.
it('should render the component', async () => { | ||
renderEditOptionList(); | ||
expect(await screen.findByText(textMock('ux_editor.options.upload_title'))).toBeInTheDocument(); | ||
}); |
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.
This test case can be done without async:
it('should render the component', async () => { | |
renderEditOptionList(); | |
expect(await screen.findByText(textMock('ux_editor.options.upload_title'))).toBeInTheDocument(); | |
}); | |
it('should render the component', () => { | |
renderEditOptionList(); | |
expect(screen.getByText(textMock('ux_editor.options.upload_title'))).toBeInTheDocument(); | |
}); |
await user.upload(fileInput, file); | ||
} | ||
|
||
async function userFindDropDownButton(user: UserEvent) { |
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.
async function userFindDropDownButton(user: UserEvent) { | |
async function userFindDropDownButtonAndClick(user: UserEvent) { |
); | ||
|
||
await userFindDropDownButton(user); | ||
const choice = screen.getByText(optionListIdsMock[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.
I think the test would be quicker to read with a more explicit variable name, for example:
const choice = screen.getByText(optionListIdsMock[0]); | |
const dropdownOption = screen.getByText(optionListIdsMock[0]); |
setChosenOption: (value: boolean) => void; | ||
} & Pick<IGenericEditComponent<SelectionComponentType>, 'component' | 'handleComponentChange'>; | ||
|
||
function DisplayChosenOption({ |
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.
Can we rename this component?
function DisplayChosenOption({ | |
function SelectedOptionList({ |
export function EditOptionList<T extends SelectionComponentType>({ | ||
setChosenOption, | ||
component, | ||
handleComponentChange, | ||
}: EditOptionListProps<T>) { | ||
const { t } = useTranslation(); | ||
const { org, app } = useStudioEnvironmentParams(); | ||
const { data: optionListIds } = useOptionListIdsQuery(org, app); | ||
const { mutate: uploadOptionList } = useAddOptionListMutation(org, app, { | ||
hideDefaultError: (error: AxiosError<ApiError>) => !error.response.data.errorCode, | ||
}); | ||
|
||
const handleOptionsIdChange = (optionsId: string) => { | ||
if (component.options) { | ||
delete component.options; | ||
} | ||
|
||
handleComponentChange({ | ||
...component, | ||
optionsId, | ||
}); | ||
|
||
setChosenOption(true); | ||
}; | ||
|
||
const onSubmit = (file: File) => { | ||
const fileNameError = findFileNameError(optionListIds, file.name); | ||
if (fileNameError) { | ||
handleInvalidFileName(fileNameError); | ||
} else { | ||
handleUpload(file); | ||
} | ||
}; | ||
|
||
const handleUpload = (file: File) => { | ||
uploadOptionList(file, { | ||
onSuccess: () => { | ||
handleOptionsIdChange(FileNameUtils.removeExtension(file.name)); | ||
toast.success(t('ux_editor.modal_properties_code_list_upload_success')); | ||
}, | ||
onError: (error: AxiosError<ApiError>) => { | ||
if (!error.response?.data?.errorCode) { | ||
toast.error(`${t('ux_editor.modal_properties_code_list_upload_generic_error')}`); | ||
} | ||
}, | ||
}); | ||
}; | ||
|
||
const handleInvalidFileName = (fileNameError: FileNameError) => { | ||
switch (fileNameError) { | ||
case 'invalidFileName': | ||
return toast.error(t('ux_editor.modal_properties_code_list_filename_error')); | ||
case 'fileExists': | ||
return toast.error(t('ux_editor.modal_properties_code_list_upload_duplicate_error')); | ||
} | ||
}; | ||
|
||
return ( | ||
<> | ||
<OptionListSelector handleOptionsIdChange={handleOptionsIdChange} /> | ||
<StudioFileUploader | ||
accept='.json' | ||
variant={'tertiary'} | ||
uploaderButtonText={t('ux_editor.options.upload_title')} | ||
onSubmit={onSubmit} | ||
/> | ||
</> | ||
); | ||
} |
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.
Can the selector and the file uploader be separated into two different components?
Suggestion:
handleOptionsIdChange
could be moved into OptionListSelector
, and a new component OptionListUploader
could wrap around StudioFileUploder
and its handler functions. This would improve separation of concerns.
Then we wouldn't need EditOptionList
anymore, and OptionListSelector
and OptionListUploader
could be called directly from EditOptionChoice
.
EditOptionChoice
could then return this:
{chosenOption ? (
<DisplayChosenOption
setChosenOption={setChosenOption}
component={component}
handleComponentChange={handleComponentChange}
/>
) : (
<div className={classes.optionButtons}>
<EditManualOptionsWithEditor
setChosenOption={setChosenOption}
component={component}
handleComponentChange={handleComponentChange}
/>
<OptionListSelector
setChosenOption={setChosenOption}
component={component}
handleComponentChange={handleComponentChange}
/>
<OptionListUploader
setChosenOption={setChosenOption}
component={component}
handleComponentChange={handleComponentChange}
/>
</div>
const btnOpen = screen.getByRole('button', { | ||
name: textMock('ux_editor.modal_properties_code_list_button_title_manual'), | ||
}); |
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.
Naming suggestion:
const btnOpen = screen.getByRole('button', { | |
name: textMock('ux_editor.modal_properties_code_list_button_title_manual'), | |
}); | |
const editOptionsButton = screen.getByRole('button', { | |
name: textMock('ux_editor.modal_properties_code_list_button_title_manual'), | |
}); |
await renderOptionListEditorAndWaitForSpinnerToBeRemoved(); | ||
|
||
await openManualModal(user); | ||
await user.click(screen.getByRole('button', { name: 'close modal' })); // Todo: Replace "close modal" with defaultDialogProps.closeButtonTitle when https://github.com/digdir/designsystemet/issues/2195 is fixed |
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.
await user.click(screen.getByRole('button', { name: 'close modal' })); // Todo: Replace "close modal" with defaultDialogProps.closeButtonTitle when https://github.com/digdir/designsystemet/issues/2195 is fixed | |
await user.click(screen.getByRole('button', { name: 'close modal' })); // Todo: Replace "close modal" with defaultDialogProps.closeButtonTitle when we upgrade to Designsystemet v1 |
it('should render the open Dialog button', async () => { | ||
await renderOptionListEditorAndWaitForSpinnerToBeRemoved(); | ||
|
||
const btnOpen = screen.getByRole('button', { |
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.
Naming suggestion:
const btnOpen = screen.getByRole('button', { | |
const editOptionsButton = screen.getByRole('button', { |
await renderOptionListEditorAndWaitForSpinnerToBeRemoved(); | ||
|
||
await openOptionModal(user); | ||
await user.click(screen.getByRole('button', { name: 'close modal' })); // Todo: Replace "close modal" with defaultDialogProps.closeButtonTitle when https://github.com/digdir/designsystemet/issues/2195 is fixed |
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.
await user.click(screen.getByRole('button', { name: 'close modal' })); // Todo: Replace "close modal" with defaultDialogProps.closeButtonTitle when https://github.com/digdir/designsystemet/issues/2195 is fixed | |
await user.click(screen.getByRole('button', { name: 'close modal' })); // Todo: Replace "close modal" with defaultDialogProps.closeButtonTitle when we upgrade to Designsystemet v1 |
const openOptionModal = async (user: UserEvent) => { | ||
const btnOpen = screen.getByRole('button', { | ||
name: textMock('ux_editor.modal_properties_code_list_button_title_library'), | ||
}); | ||
await user.click(btnOpen); | ||
}; | ||
const openManualModal = async (user: UserEvent) => { | ||
const btnOpen = screen.getByRole('button', { | ||
name: textMock('ux_editor.modal_properties_code_list_button_title_manual'), | ||
}); | ||
await user.click(btnOpen); | ||
}; |
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.
Naming suggestion:
const openOptionModal = async (user: UserEvent) => { | |
const btnOpen = screen.getByRole('button', { | |
name: textMock('ux_editor.modal_properties_code_list_button_title_library'), | |
}); | |
await user.click(btnOpen); | |
}; | |
const openManualModal = async (user: UserEvent) => { | |
const btnOpen = screen.getByRole('button', { | |
name: textMock('ux_editor.modal_properties_code_list_button_title_manual'), | |
}); | |
await user.click(btnOpen); | |
}; | |
const openOptionModal = async (user: UserEvent) => { | |
const editOptionsButton = screen.getByRole('button', { | |
name: textMock('ux_editor.modal_properties_code_list_button_title_library'), | |
}); | |
await user.click(editOptionsButton); | |
}; | |
const openManualModal = async (user: UserEvent) => { | |
const editOptionsButton = screen.getByRole('button', { | |
name: textMock('ux_editor.modal_properties_code_list_button_title_manual'), | |
}); | |
await user.click(editOptionsButton); | |
}; |
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.
Edited to not have user.click inside the functions:
function getOptionModalButton() {
return screen.getByRole('button', {
name: textMock('ux_editor.modal_properties_code_list_button_title_library'),
});
}
function getManualModalButton() {
return screen.getByRole('button', {
name: textMock('ux_editor.modal_properties_code_list_button_title_manual'),
});
}
return ( | ||
<OptionListEditorModal | ||
label={label} | ||
optionsId={optionsId} | ||
optionsList={optionsListMap[optionsId]} | ||
/> | ||
); | ||
} | ||
if (component.options !== undefined) { | ||
return ( | ||
<OptionListEditorModalManualOptions | ||
label={label} | ||
component={component} | ||
handleComponentChange={handleComponentChange} | ||
/> |
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.
Naming suggestion:
return ( | |
<OptionListEditorModal | |
label={label} | |
optionsId={optionsId} | |
optionsList={optionsListMap[optionsId]} | |
/> | |
); | |
} | |
if (component.options !== undefined) { | |
return ( | |
<OptionListEditorModalManualOptions | |
label={label} | |
component={component} | |
handleComponentChange={handleComponentChange} | |
/> | |
return ( | |
<LibraryOptionListEditorModal | |
label={label} | |
optionsId={optionsId} | |
optionsList={optionsListMap[optionsId]} | |
/> | |
); | |
} | |
if (component.options !== undefined) { | |
return ( | |
<ManualOptionListEditorModal | |
label={label} | |
component={component} | |
handleComponentChange={handleComponentChange} | |
/> |
const { org, app } = useStudioEnvironmentParams(); | ||
const { doReloadPreview } = usePreviewContext(); | ||
const { mutate: updateOptionList } = useUpdateOptionListMutation(org, app); | ||
const { debounce } = useDebounce({ debounceTimeInMs: AUTOSAVE_DEBOUNCE_INTERVAL_MILLISECONDS }); |
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 debounce, now that we save onBlur?
...onents/config/editModal/EditOptions/OptionTabs/EditOptionList-v1/EditOptionListReference.tsx
Show resolved
Hide resolved
describe('EditOptions', () => { | ||
afterEach(() => jest.clearAllMocks()); | ||
|
||
it('should render component', async () => { |
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('should render component', async () => { | |
it('should render component', () => { |
expect(screen.getByText(textMock('ux_editor.options.tab_code_list'))).toBeInTheDocument(); | ||
}); | ||
|
||
it('should show code list input by default when neither options nor optionId are set', async () => { |
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.
Nice test case! But async can be removed:
it('should show code list input by default when neither options nor optionId are set', async () => { | |
it('should show code list input by default when neither options nor optionId are set', () => { |
).toBeInTheDocument(); | ||
}); | ||
|
||
it('should show code list tab when component has optionsId defined matching an optionId in optionsID-list', async () => { |
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('should show code list tab when component has optionsId defined matching an optionId in optionsID-list', async () => { | |
it('should show code list tab when component has optionsId defined matching an optionId in optionsID-list', () => { |
).toBeInTheDocument(); | ||
}); | ||
|
||
it('should show referenceId tab when component has optionsId defined not matching an optionId in optionsId-list', async () => { |
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('should show referenceId tab when component has optionsId defined not matching an optionId in optionsId-list', async () => { | |
it('should show referenceId tab when component has optionsId defined not matching an optionId in optionsId-list', () => { |
).toBeInTheDocument(); | ||
}); | ||
|
||
it('should switch to code list tab when input clicking code list tab', async () => { |
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('should switch to code list tab when input clicking code list tab', async () => { | |
it('should switch to code list tab when clicking code list tab', async () => { |
) : ( | ||
<EditManualOptions component={component} handleComponentChange={handleComponentChange} /> | ||
<EditOptionChoice component={component} handleComponentChange={handleComponentChange} /> | ||
{errorMessage && component.options !== 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.
component.options !== undefined
Is this because Det må være minst én radioknapp
shows up too often?
Maybe we can keep the error message for now (since we are behind feature flag), and update the logic in useComponentErrorMessage/useValidateComponent
in a separate issue?
// Todo: Remove once featureFlag "optionListEditor" is removed. | ||
type RenderManualOptionsV1Props = { | ||
areLayoutOptionsSupported: boolean; | ||
} & Pick<IGenericEditComponent<SelectionComponentType>, 'component' | 'handleComponentChange'>; |
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 we will avoid having these "almost duplicate" functions in the same files, if we do a hard separation of the old and new features as suggested in another comment 🤔
// Copy of function above. Todo: Remove once featureFlag "optionListEditor" is removed. | ||
export function getSelectedOptionsTypeV1( | ||
codeListId: string | undefined, | ||
options: IOption[] | undefined, | ||
optionListIds: string[] = [], | ||
): SelectedOptionsType { | ||
/** It is not permitted for a component to have both options and optionsId set on the same component. */ | ||
if (options?.length && codeListId) { | ||
return SelectedOptionsType.Unknown; | ||
} |
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.
Can getSelectedOptionsTypeV1
and getSelectedOptionsType
be moved next to where they are used? If we can place getSelectedOptionsTypeV1
inside one of the "old" folders, Then we don't need to have different versions next to each other, and the old function will be deleted together with the old functionality.
Discovered this bug when having duplicate values: Spiller.inn.2024-11-26.153820.mp4 |
Description
The PR contains the following:
CodeList
andManual
tabs in configonChange
->onBlur
for StudioCodeListEditorcodelist
->code_list
It's possible to split it into multiple PRs to make Review & Testing easier, but i'll leave that up to the person doing review.
Video of current design
PR.13685.mp4
Duplicated files
There are a few duplicate files and functions in this PR. These have been marked with
Todo: Remove
comments, or are in a seperate folder. The duplicates have been created to make it easier to remove old code once we're removing the feature flagoptionListEditor
.Localtions of duplicate code:
OptionListEditor-v1
folderOptionTabs.tsx
OptionUtils.ts
Related Issue(s)
Verification