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

Cscfc4 emscr 591 remove file upload from add revision modal for mscr format #230

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

rquazi
Copy link
Member

@rquazi rquazi commented Nov 11, 2024

If the schema format is MSCR, then the revision creation modal should not have the file upload or URI input box.

@rquazi rquazi requested a review from maariaw November 11, 2024 10:52
@rquazi rquazi assigned masillan and unassigned masillan Nov 11, 2024
@rquazi rquazi requested a review from masillan November 11, 2024 10:53
Copy link

@maariaw maariaw left a comment

Choose a reason for hiding this comment

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

Nice! A couple minor notes, but everything works. Can be merged after fixing at least the todo that can be removed and the conditional operation.

@@ -108,12 +111,12 @@ export default function FormModal({
usePutSchemaMscrCopyMutation();
const [submitAnimationVisible, setSubmitAnimationVisible] =
useState<boolean>(false);

Copy link

Choose a reason for hiding this comment

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

Extra whitespace

const formDataFromInitialData = useCallback(() => {
if (!initialData) return;
const existingData: FormType = {
format:
modalType == ModalType.MscrCopy ? Format.Mscr : initialData.format,
modalType == ModalType.MscrCopy? Format.Mscr : initialData.format,
Copy link

Choose a reason for hiding this comment

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

There should be a space between the conditional operators and the operands, so restore space between 'ModalType.MscrCopy' and '?'

@@ -352,6 +364,7 @@ export default function FormModal({
} else if (formData.format !== Format.Mscr) {
return;
}

Copy link

Choose a reason for hiding this comment

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

Accidental?

@@ -363,7 +376,7 @@ export default function FormModal({
setSubmitAnimationVisible(false);
}
);
} else if (modalType == ModalType.RegisterNewMscr) {
} else if (modalType == ModalType.RegisterNewMscr) {//what is register new MSCR?
Copy link

Choose a reason for hiding this comment

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

If you look at the definition of the ModalType:

export enum ModalType {
  RegisterNewFull = 'REGISTER_NEW_FULL',
  RegisterNewMscr = 'REGISTER_NEW_MSCR',
  RevisionFull = 'REVISION_FULL',
  RevisionMscr = 'REVISION_MSCR',
  MscrCopy = 'MSCR_COPY',
}

RegisterNewMscr is registering something in MSCR-format, where there's no file sent, as opposed to RegisterNewFull, where there should also be a file attached. In practice it always means creating (and registering) a new crosswalk, since that's always MSCR-format.

Copy link
Member Author

@rquazi rquazi Nov 11, 2024

Choose a reason for hiding this comment

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

Thanks :) I was wondering what this action :)

@@ -7,6 +7,7 @@ import {
formatsAvailableForSchemaRegistration,
} from '@app/common/interfaces/format.interface';


Copy link

Choose a reason for hiding this comment

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

Is there a reason for adding extra empty lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, just accidental !!

@@ -352,15 +352,28 @@ export default function SchemaView({ schemaId }: { schemaId: string }) {
/>
)}
{/*ToDo: When making a revision of an mscr copy is possible, take that into account here (Modaltype.RevisionMscr)*/}
Copy link

Choose a reason for hiding this comment

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

This ToDo is completed with your changes! :)

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