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

feat: add codeListEditor in config options #13953

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Konrad-Simso
Copy link
Contributor

Description

Added StudioCodeListEditor component to options config, through new component named CodeListTableEditor.
The component is hidden behind the featureFlag codeListEditor, as we do not wish to change prod before studio supports multiple types, and merging the view for choosing or editing codelists into 1 tab.

Video

Skjermopptak.2024-10-30.142753.mp4

Out of scope:

  • Update types for backend & frontend for options/option lists.

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)

Documentation

@github-actions github-actions bot added area/ui-editor Area: Related to the designer tool for assembling app UI in Altinn Studio. area/dashboard Area: Related to the dashboard solution/studio/designer Issues related to the Altinn Studio Designer solution. frontend labels Oct 30, 2024
Copy link
Contributor

@standeren standeren left a comment

Choose a reason for hiding this comment

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

Nice work 🎉
Some small questions and comments in general, and also a suggestion to adapt renderWithProviders to make the tests more readable 🤗

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 83.09859% with 12 lines in your changes missing coverage. Please review.

Project coverage is 95.25%. Comparing base (4333644) to head (9e70770).
Report is 44 commits behind head on main.

Files with missing lines Patch % Lines
...tModal/EditOptions/EditCodeList/CodeListEditor.tsx 86.48% 4 Missing and 1 partial ⚠️
...tions/EditCodeList/hooks/useCodeListEditorTexts.ts 44.44% 5 Missing ⚠️
...ditModal/EditOptions/EditCodeList/EditCodeList.tsx 88.88% 0 Missing and 1 partial ⚠️
...ux-editor/src/hooks/queries/useOptionListsQuery.ts 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13953      +/-   ##
==========================================
- Coverage   95.29%   95.25%   -0.04%     
==========================================
  Files        1653     1656       +3     
  Lines       22024    22081      +57     
  Branches     2589     2594       +5     
==========================================
+ Hits        20988    21034      +46     
- Misses        790      798       +8     
- Partials      246      249       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@standeren standeren left a comment

Choose a reason for hiding this comment

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

Much better ⭐ I still have an suggestion to avoid the useEffect and undefined checks in CodeListEditor.tsx 🙈

import { useOptionListsQuery } from '../../../../../hooks/queries/useOptionListsQuery';
import { useCodeListEditorTexts } from './hooks/useCodeListEditorTexts';
import { usePreviewContext } from 'app-development/contexts/PreviewContext';
import { ErrorMessage } from '@digdir/designsystemet-react';
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a StudioErrorMessage now 🤗

setCodeList(options);
};

if (component.optionsId === undefined) return <StudioSpinner spinnerTitle={'test'} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Guess this spinnerTitle should not be test 🙈
And do we need it? Why should there be a indefinite spinner when there is no optionsId connected to the component? As it is okey for a component not to have a connected optionList

if (component.optionsId === undefined) return <StudioSpinner spinnerTitle={'test'} />;
switch (status) {
case 'pending':
return <StudioSpinner spinnerTitle={'test'} />; // Extract title to nb.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above 🙈 I see you have reminded yourself as well

case 'error':
return (
<ErrorMessage>
{error instanceof Error ? error.message : t('ux_editor.modal_properties_error_message')}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we ever expose the backend error to the user? 🤔 I think it is okay to display a generic error in this iteration, and then we can potentially implement a better error handling in another issue that uses text-keys for errors received from api 🤗

Copy link
Contributor

Choose a reason for hiding this comment

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

Two comments here:

  1. We only need to pass queries and the queryclient if we are modifying them. They will be defined in the renderWithProviders if they are not passed 🤗 As this test is now it does not look like it is modifying the queryClients cache so we dont need to pass it.
  2. I see that you await for the spinner to be removed. I see two different approaches here;
    2.1: we can set the optionList directly on the cache if we anyway always will wait for the spinner to removed before executing the tests.
    2.2: we can actually test that the spinner is there when it suppose to be. There are also two ways to implement that. Either you can set the cache in the renderMethod if data is passed to the renderMethod. Or you can use adapt the queries and have the waitForSpinnerTobeRemoved in a separete renderMethod. We have examples of both approcahes around the codebase. Let me know if you need help to find it 🤗

previewContextProps: previewContextProps,
},
);
await waitForElementToBeRemoved(screen.queryByTestId('studio-spinner-test-id'));
Copy link
Contributor

Choose a reason for hiding this comment

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

If you decide to keep the waitForElementToBeRemoved method we should use the text on it instead of test Id I think 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Okey, I have a suggestion to simplify this code by separating a bit more. It will be too difficult to describe my suggestion so I will simply paste an updated version of the file:

import React, { createRef, useState } from 'react';
import type { IGenericEditComponent } from '@altinn/ux-editor/components/config/componentConfig';
import type { SelectionComponentType } from '@altinn/ux-editor/types/FormComponent';
import type { Option } from 'app-shared/types/Option';
import type { ApiError } from 'app-shared/types/api/ApiError';
import type { AxiosError } from 'axios';
import { useTranslation } from 'react-i18next';
import {
  StudioCodeListEditor,
  StudioModal,
  StudioSpinner,
  type CodeList,
} from '@studio/components';
import { TableIcon } from '@studio/icons';
import { useStudioEnvironmentParams } from 'app-shared/hooks/useStudioEnvironmentParams';
import { useUpdateOptionListMutation } from '../../../../../hooks/mutations/useUpdateOptionListMutation';
import { useOptionListsQuery } from '../../../../../hooks/queries/useOptionListsQuery';
import { useCodeListEditorTexts } from './hooks/useCodeListEditorTexts';
import { usePreviewContext } from 'app-development/contexts/PreviewContext';
import { ErrorMessage } from '@digdir/designsystemet-react';
import classes from './CodeListEditor.module.css';

type CodeListEditorProps = Pick<IGenericEditComponent<SelectionComponentType>, 'component'>;

export function CodeListEditor({ component }: CodeListEditorProps): React.ReactNode {
  const { t } = useTranslation();
  const { org, app } = useStudioEnvironmentParams();
  const { data: optionsListMap, status, error } = useOptionListsQuery(org, app);
  
  switch (status) {
    case 'pending':
      return <StudioSpinner spinnerTitle={'test'} />; // Extract title to nb.json
    case 'error':
      return (
        <ErrorMessage>
          {t('ux_editor.modal_properties_error_message')}
        </ErrorMessage>
      );
    case 'success': {
      return (
        <CodeListEditorModal
          codeList={optionsListMap[component.optionsId]}
          component={component}
        />
      );
    }
  }
}

type CodeListEditorModalProps = {
  codeList: CodeList;
} & Pick<IGenericEditComponent<SelectionComponentType>, 'component'>;

function CodeListEditorModal({
  codeList,
  component,
}: CodeListEditorModalProps): React.ReactNode {
  const { t } = useTranslation();
  const { org, app } = useStudioEnvironmentParams();
  const { doReloadPreview } = usePreviewContext();
  const { mutate: uploadOptionList } = useUpdateOptionListMutation(org, app, {
    hideDefaultError: (apiError: AxiosError<ApiError>) => !apiError.response.data.errorCode,
  });
  const editorTexts = useCodeListEditorTexts();
  const modalRef = createRef<HTMLDialogElement>();
  const [currentCodeList, setCodeList] = useState<CodeList>(codeList);

  const handleOptionsChange = (options?: Option[]) => {
    setCodeList(options);
  };

  const handleClose = () => {
    uploadOptionList({ optionListId: component.optionsId, optionsList: currentCodeList });
    doReloadPreview();
    modalRef.current?.close();
  };

  return (
    <StudioModal.Root>
      <StudioModal.Trigger
        className={classes.modalTrigger}
        variant='secondary'
        icon={<TableIcon />}
      >
        {t('ux_editor.modal_properties_code_list_open_editor')}
      </StudioModal.Trigger>
      <StudioModal.Dialog
        ref={modalRef}
        className={classes.manualTabModal}
        closeButtonTitle={t('general.close')}
        heading={t('ux_editor.modal_add_options_codelist')}
        onBeforeClose={handleClose}
        onInteractOutside={handleClose}
      >
        <StudioCodeListEditor
          codeList={currentCodeList}
          onChange={handleOptionsChange}
          texts={editorTexts}
        />
      </StudioModal.Dialog>
    </StudioModal.Root>
  );
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a test here that checks if the feature is available if the feature is enabled? 🧐

renderOptions={renderOptions}
/>,
{
queries,
queries: queries,
Copy link
Contributor

Choose a reason for hiding this comment

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

Dont think this is necessary 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dashboard Area: Related to the dashboard area/ui-editor Area: Related to the designer tool for assembling app UI in Altinn Studio. frontend solution/studio/designer Issues related to the Altinn Studio Designer solution.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to use manual editor to edit predefined codelists
2 participants