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: Create collection Modal [FC-0062] #1259

Merged
merged 17 commits into from
Sep 14, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/library-authoring/LibraryLayout.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import React from 'react';
import LibraryAuthoringPage from './LibraryAuthoringPage';
import { LibraryProvider } from './common/context';
import { CreateCollectionModal } from './create-collection';

const LibraryLayout = () => (
<LibraryProvider>
<LibraryAuthoringPage />
<CreateCollectionModal />
</LibraryProvider>
);

Expand Down
65 changes: 47 additions & 18 deletions src/library-authoring/add-content/AddContentContainer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,40 @@ import { getCanEdit } from '../../course-unit/data/selectors';
import { useCreateLibraryBlock, useLibraryPasteClipboard } from '../data/apiHooks';

import messages from './messages';
import { LibraryContext } from '../common/context';

type ContentType = {
name: string,
disabled: boolean,
icon: React.ComponentType,
blockType: string,
};

type AddContentButtonProps = {
contentType: ContentType,
onCreateContent: (blockType: string) => void,
};

const AddContentButton = ({ contentType, onCreateContent } : AddContentButtonProps) => {
const {
name,
disabled,
icon,
blockType,
} = contentType;
return (
<Button
key={`add-content-${blockType}`}
ChrisChV marked this conversation as resolved.
Show resolved Hide resolved
variant="outline-primary"
disabled={disabled}
className="m-2 rounded-0"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more of a general comment (not related to this PR).
I don't think we should set rounded-0 here (and I don't think it should be that way in the design).

@bradenmacdonald, should we ask the design team to use the default Paragon theme? It is hard to be consistent in manually implementing these overrides, and if we hardcode these styles, we prevent the theme from being applied properly in my opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - in general, please just use the Paragon styles and don't override them unless there's a very specific reason required for the design. That way the components will be consistent, and it also makes development faster. This is also an MVP so we're not generally aiming for pixel-perfect UX design.

@sdaitzman @lizc577 can I get your thoughts - do you have a Paragon theme for Figma now?

Copy link

@sdaitzman sdaitzman Sep 10, 2024

Choose a reason for hiding this comment

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

Which border radius is this about? Edit: I looked through the TypeScript, if I'm correct this is about the Add Content button: no need to override the Paragon theme border radius.

Most components and workflows in Figma are using Paragon Figma components (a few screens haven't been updated, and there are occasional overrides to fix UX issues in the original Paragon components).

The Paragon Figma library is based on the 2U/EdX theme, so we don't expect the implemented components to match exactly. The UX/UI and Paragon WGs have been discussing building an Open EdX Paragon Figma library, but only a small fraction exists right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification. Updated here: b0a47cc

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your input here @sdaitzman!
image
image

We have some buttons for which we overrode the radius to match the design and some for which we did not.
From now on, I will not override the Paragon styles.

If you catch some inconsistencies in the UX/UI validation, please let us know!!

iconBefore={icon}
onClick={() => onCreateContent(blockType)}
>
{name}
</Button>
);
};

const AddContentContainer = () => {
const intl = useIntl();
Expand All @@ -32,7 +66,16 @@ const AddContentContainer = () => {
const { showToast } = useContext(ToastContext);
const canEdit = useSelector(getCanEdit);
const { showPasteXBlock } = useCopyToClipboard(canEdit);
const {
openCreateCollectionModal,
} = React.useContext(LibraryContext);

const collectionButtonData = {
name: intl.formatMessage(messages.collectionButton),
disabled: false,
icon: BookOpen,
blockType: 'collection',
};
const contentTypes = [
{
name: intl.formatMessage(messages.textTypeButton),
Expand Down Expand Up @@ -95,6 +138,8 @@ const AddContentContainer = () => {
}).catch(() => {
showToast(intl.formatMessage(messages.errorPasteClipboardMessage));
});
} else if (blockType === 'collection') {
openCreateCollectionModal();
} else {
createBlockMutation.mutateAsync({
libraryId,
Expand All @@ -115,26 +160,10 @@ const AddContentContainer = () => {

return (
<Stack direction="vertical">
<Button
variant="outline-primary"
disabled
className="m-2 rounded-0"
iconBefore={BookOpen}
>
{intl.formatMessage(messages.collectionButton)}
</Button>
<AddContentButton contentType={collectionButtonData} onCreateContent={onCreateContent} />
<hr className="w-100 bg-gray-500" />
{contentTypes.map((contentType) => (
<Button
key={`add-content-${contentType.blockType}`}
variant="outline-primary"
disabled={contentType.disabled}
className="m-2 rounded-0"
iconBefore={contentType.icon}
onClick={() => onCreateContent(contentType.blockType)}
>
{contentType.name}
</Button>
<AddContentButton contentType={contentType} onCreateContent={onCreateContent} />
ChrisChV marked this conversation as resolved.
Show resolved Hide resolved
))}
</Stack>
);
Expand Down
14 changes: 14 additions & 0 deletions src/library-authoring/common/context.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* eslint-disable react/require-default-props */
import { useToggle } from '@openedx/paragon';
import React from 'react';

export enum SidebarBodyComponentId {
Expand All @@ -14,6 +15,9 @@ export interface LibraryContextData {
openInfoSidebar: () => void;
openComponentInfoSidebar: (usageKey: string) => void;
currentComponentUsageKey?: string;
isCreateCollectionModalOpen: boolean;
openCreateCollectionModal: () => void;
closeCreateCollectionModal: () => void;
}

export const LibraryContext = React.createContext({
Expand All @@ -22,6 +26,9 @@ export const LibraryContext = React.createContext({
openAddContentSidebar: () => {},
openInfoSidebar: () => {},
openComponentInfoSidebar: (_usageKey: string) => {}, // eslint-disable-line @typescript-eslint/no-unused-vars
isCreateCollectionModalOpen: false,
openCreateCollectionModal: () => {},
closeCreateCollectionModal: () => {},
} as LibraryContextData);

/**
Expand All @@ -30,6 +37,7 @@ export const LibraryContext = React.createContext({
export const LibraryProvider = (props: { children?: React.ReactNode }) => {
const [sidebarBodyComponent, setSidebarBodyComponent] = React.useState<SidebarBodyComponentId | null>(null);
const [currentComponentUsageKey, setCurrentComponentUsageKey] = React.useState<string>();
const [isCreateCollectionModalOpen, openCreateCollectionModal, closeCreateCollectionModal] = useToggle(false);

const closeLibrarySidebar = React.useCallback(() => {
setSidebarBodyComponent(null);
Expand Down Expand Up @@ -58,13 +66,19 @@ export const LibraryProvider = (props: { children?: React.ReactNode }) => {
openInfoSidebar,
openComponentInfoSidebar,
currentComponentUsageKey,
isCreateCollectionModalOpen,
openCreateCollectionModal,
closeCreateCollectionModal,
}), [
sidebarBodyComponent,
closeLibrarySidebar,
openAddContentSidebar,
openInfoSidebar,
openComponentInfoSidebar,
currentComponentUsageKey,
isCreateCollectionModalOpen,
openCreateCollectionModal,
closeCreateCollectionModal,
]);

return (
Expand Down
134 changes: 134 additions & 0 deletions src/library-authoring/create-collection/CreateCollectionModal.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
import React from 'react';
import {
ActionRow,
Button,
Form,
ModalDialog,
} from '@openedx/paragon';
import { useIntl } from '@edx/frontend-platform/i18n';
import { useParams } from 'react-router-dom';
import { LibraryContext } from '../common/context';
import messages from './messages';
import { useCreateLibraryCollection } from '../data/apiHooks';
import { ToastContext } from '../../generic/toast-context';

const CreateCollectionModal = () => {
const intl = useIntl();
const { libraryId } = useParams();
const create = useCreateLibraryCollection(libraryId);
const {
isCreateCollectionModalOpen,
closeCreateCollectionModal,
} = React.useContext(LibraryContext);
const { showToast } = React.useContext(ToastContext);

const [collectionName, setCollectionName] = React.useState<string | null>(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not use null as a value on an input. It should be '' for a controlled component. This generates some warnings in the console.

Suggested change
const [collectionName, setCollectionName] = React.useState<string | null>(null);
const [collectionName, setCollectionName] = React.useState<string>('');

Also, I think it would be better to use Formik + Yup for form validation and state management instead of doing it manually. We have a recent example here: https://github.com/openedx/frontend-app-course-authoring/blob/master/src/library-authoring/create-library/CreateLibrary.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated: aa870b4

const [collectionNameInvalidMsg, setCollectionNameInvalidMsg] = React.useState<string | null>(null);
const [collectionDescription, setCollectionDescription] = React.useState<string | null>(null);
const [isCreatingCollection, setIsCreatingCollection] = React.useState<boolean>(false);

const handleNameOnChange = React.useCallback((value : string) => {
setCollectionName(value);
setCollectionNameInvalidMsg(null);
}, []);

const handleOnClose = React.useCallback(() => {
closeCreateCollectionModal();
setCollectionNameInvalidMsg(null);
setCollectionName(null);
setCollectionDescription(null);
setIsCreatingCollection(false);
}, []);

const handleCreate = React.useCallback(() => {
if (collectionName === null || collectionName === '') {
setCollectionNameInvalidMsg(
intl.formatMessage(messages.createCollectionModalNameInvalid),
);
return;
}

setIsCreatingCollection(true);

create.mutateAsync({
title: collectionName,
description: collectionDescription || '',
}).then(() => {
handleOnClose();
showToast(intl.formatMessage(messages.createCollectionSuccess));
}).catch((err) => {
setIsCreatingCollection(false);
if (err.customAttributes.httpErrorStatus === 409) {
setCollectionNameInvalidMsg(
intl.formatMessage(messages.createCollectionModalNameConflict),
);
} else {
showToast(intl.formatMessage(messages.createCollectionError));
}
});
}, [collectionName, collectionDescription]);

return (
<ModalDialog
title={intl.formatMessage(messages.createCollectionModalTitle)}
isOpen={isCreateCollectionModalOpen}
onClose={handleOnClose}
ChrisChV marked this conversation as resolved.
Show resolved Hide resolved
size="xl"
hasCloseButton
isFullscreenOnMobile
>
<ModalDialog.Header>
<ModalDialog.Title>
{intl.formatMessage(messages.createCollectionModalTitle)}
</ModalDialog.Title>
</ModalDialog.Header>

<ModalDialog.Body className="mw-sm">
<Form>
<Form.Group>
<Form.Label className="font-weight-bold h3">
{intl.formatMessage(messages.createCollectionModalNameLabel)}
</Form.Label>
<Form.Control
placeholder={intl.formatMessage(messages.createCollectionModalNamePlaceholder)}
value={collectionName}
onChange={(e) => handleNameOnChange(e.target.value)}
/>
{ collectionNameInvalidMsg && (
<Form.Control.Feedback type="invalid">
{collectionNameInvalidMsg}
</Form.Control.Feedback>
)}
</Form.Group>
<Form.Group>
<Form.Label className="font-weight-bold h3">
{intl.formatMessage(messages.createCollectionModalDescriptionLabel)}
</Form.Label>
<Form.Control
as="textarea"
placeholder={intl.formatMessage(messages.createCollectionModalDescriptionPlaceholder)}
value={collectionDescription}
onChange={(e) => setCollectionDescription(e.target.value)}
rows="5"
/>
<Form.Text muted>
{intl.formatMessage(messages.createCollectionModalDescriptionDetails)}
</Form.Text>
</Form.Group>
</Form>
</ModalDialog.Body>
<ModalDialog.Footer>
<ActionRow>
<ModalDialog.CloseButton variant="tertiary">
{intl.formatMessage(messages.createCollectionModalCancel)}
</ModalDialog.CloseButton>
<Button variant="primary" onClick={handleCreate} disabled={isCreatingCollection}>
{intl.formatMessage(messages.createCollectionModalCreate)}
</Button>
</ActionRow>
</ModalDialog.Footer>
</ModalDialog>
);
};

export default CreateCollectionModal;
2 changes: 2 additions & 0 deletions src/library-authoring/create-collection/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// eslint-disable-next-line import/prefer-default-export
export { default as CreateCollectionModal } from './CreateCollectionModal';
70 changes: 70 additions & 0 deletions src/library-authoring/create-collection/messages.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import { defineMessages as _defineMessages } from '@edx/frontend-platform/i18n';
import type { defineMessages as defineMessagesType } from 'react-intl';

// frontend-platform currently doesn't provide types... do it ourselves.
const defineMessages = _defineMessages as typeof defineMessagesType;

const messages = defineMessages({
createCollectionModalTitle: {
id: 'course-authoring.library-authoring.modals.create-collection.title',
defaultMessage: 'New Collection',
description: 'Title of the Create Collection modal',
},
createCollectionModalCancel: {
id: 'course-authoring.library-authoring.modals.create-collection.cancel',
defaultMessage: 'Cancel',
description: 'Label of the Cancel button of the Create Collection modal',
},
createCollectionModalCreate: {
id: 'course-authoring.library-authoring.modals.create-collection.create',
defaultMessage: 'Create',
description: 'Label of the Create button of the Create Collection modal',
},
createCollectionModalNameLabel: {
id: 'course-authoring.library-authoring.modals.create-collection.form.name',
defaultMessage: 'Name your collection',
description: 'Label of the Name field of the Create Collection modal form',
},
createCollectionModalNamePlaceholder: {
id: 'course-authoring.library-authoring.modals.create-collection.form.name.placeholder',
defaultMessage: 'Give a descriptive title',
description: 'Placeholder of the Name field of the Create Collection modal form',
},
createCollectionModalNameInvalid: {
id: 'course-authoring.library-authoring.modals.create-collection.form.name.invalid',
defaultMessage: 'Collection name is required',
description: 'Mesasge when the Name field of the Create Collection modal form is invalid',
ChrisChV marked this conversation as resolved.
Show resolved Hide resolved
},
createCollectionModalNameConflict: {
id: 'course-authoring.library-authoring.modals.create-collection.form.name.conflict',
defaultMessage: 'There is another collection with the same name',
description: 'Mesasge when the Name field of the Create Collection modal form is not unique',
ChrisChV marked this conversation as resolved.
Show resolved Hide resolved
},
createCollectionModalDescriptionLabel: {
id: 'course-authoring.library-authoring.modals.create-collection.form.description',
defaultMessage: 'Add a description (optional)',
description: 'Label of the Description field of the Create Collection modal form',
},
createCollectionModalDescriptionPlaceholder: {
id: 'course-authoring.library-authoring.modals.create-collection.form.description.placeholder',
defaultMessage: 'Add description',
description: 'Placeholder of the Description field of the Create Collection modal form',
},
createCollectionModalDescriptionDetails: {
id: 'course-authoring.library-authoring.modals.create-collection.form.description.details',
defaultMessage: 'Descriptions can help you and your team better organize and find what you are looking for',
description: 'Details of the Description field of the Create Collection modal form',
},
createCollectionSuccess: {
id: 'course-authoring.library-authoring.modals.create-collection.success',
defaultMessage: 'Collection created successfully',
description: 'Success message when creating a library collection',
},
createCollectionError: {
id: 'course-authoring.library-authoring.modals.create-collection.error',
defaultMessage: 'There is an error when creating the library collection',
description: 'Error message when creating a library collection',
},
});

export default messages;
Loading
Loading