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

Conversation

ChrisChV
Copy link
Contributor

@ChrisChV ChrisChV commented Sep 5, 2024

Description

Adds a functional Modal to create a library collection.

image

Supporting information

Testing instructions

  • Use this branch of edx-platform: Update collections crud rest api open-craft/edx-platform#683
  • Go to studio and create a library
  • In the library home click on the New button
  • Click the' Collection' button on the Add content sidebar.
  • Verify the design of the modal.
  • Click on Create with all empty. Verify the validation error.
  • Add a name and click on Create. Verify the success.
  • Run tutor dev run cms ./manage.py cms reindex_studio --experimental and refresh the library home page to see the newly created collection.
  • Return to the create modal and add the same name of the newly created collection. Click on Create and verify the validation error.

@ChrisChV ChrisChV requested a review from a team as a code owner September 5, 2024 22:55
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 5, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Sep 5, 2024

Thanks for the pull request, @ChrisChV!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-tnl. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@ChrisChV ChrisChV marked this pull request as draft September 5, 2024 22:56
@ChrisChV ChrisChV changed the title Create collection Modal feat: Create collection Modal [FC-0062] Sep 5, 2024
Copy link

codecov bot commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 95.94595% with 3 lines in your changes missing coverage. Please review.

Project coverage is 92.32%. Comparing base (fd48fef) to head (8dfe82c).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/library-authoring/common/context.tsx 50.00% 2 Missing ⚠️
...horing/create-collection/CreateCollectionModal.tsx 96.55% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1259      +/-   ##
==========================================
+ Coverage   92.31%   92.32%   +0.01%     
==========================================
  Files        1013     1016       +3     
  Lines       18733    18803      +70     
  Branches     3947     4017      +70     
==========================================
+ Hits        17293    17360      +67     
  Misses       1374     1374              
- Partials       66       69       +3     

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

@mphilbrick211 mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Sep 6, 2024
@ChrisChV ChrisChV marked this pull request as ready for review September 7, 2024 19:28
Copy link
Contributor

@rpenido rpenido left a comment

Choose a reason for hiding this comment

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

This is looking good to me @ChrisChV!
I left some comments. Let me know what you think!

src/library-authoring/add-content/AddContentContainer.tsx Outdated Show resolved Hide resolved
src/library-authoring/add-content/AddContentContainer.tsx Outdated Show resolved Hide resolved
src/library-authoring/create-collection/messages.ts Outdated Show resolved Hide resolved
src/library-authoring/create-collection/messages.ts Outdated Show resolved Hide resolved
src/library-authoring/data/apiHooks.ts Outdated Show resolved Hide resolved
src/library-authoring/data/apiHooks.ts Outdated Show resolved Hide resolved
src/library-authoring/data/apiHooks.ts Outdated Show resolved Hide resolved
src/library-authoring/data/api.ts Outdated Show resolved Hide resolved
key={`add-content-${blockType}`}
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!!

} = 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

Copy link
Contributor

@rpenido rpenido left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Thank you for your work, @ChrisChV!
It worked flawlessly.

I added some optional suggestions about using the formik, but feel free to push back if you want to!

  • I tested this using the instructions from the PR
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

src/library-authoring/data/apiHooks.ts Outdated Show resolved Hide resolved
Comment on lines 28 to 34
const [isCreatingCollection, setIsCreatingCollection] = React.useState<boolean>(false);

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Then this can go

Suggested change
const [isCreatingCollection, setIsCreatingCollection] = React.useState<boolean>(false);
const handleOnClose = React.useCallback(() => {
closeCreateCollectionModal();
setIsCreatingCollection(false);
}, []);

)}
value={formikProps.values.title}
placeholder={intl.formatMessage(messages.createCollectionModalNamePlaceholder)}
help=""
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit

Suggested change
help=""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

I get this error

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand.. help has a defaultProp -- why isn't it ok to remove it?

But if it must be included, then just send an empty element so we don't have to change the expected types:

Suggested change
help=""
help=<></>

Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

Hi @ChrisChV , this is working well, but I have some minor comments. Other than removing the unused message, none of my comments need to block merging, though, so:

👍

Comment on lines 44 to 45
label: PropTypes.oneOfType([PropTypes.string, PropTypes.element]),
help: PropTypes.oneOfType([PropTypes.string, PropTypes.element]),
Copy link
Contributor

Choose a reason for hiding this comment

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

label is still always a PropTypes.element.. and help would be if you just wrapped it in a <Form.Text> element.
So rather than making this change, can we please just call it with elements instead of strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

)}
value={formikProps.values.title}
placeholder={intl.formatMessage(messages.createCollectionModalNamePlaceholder)}
help=""
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand.. help has a defaultProp -- why isn't it ok to remove it?

But if it must be included, then just send an empty element so we don't have to change the expected types:

Suggested change
help=""
help=<></>

)}
value={formikProps.values.description}
placeholder={intl.formatMessage(messages.createCollectionModalDescriptionPlaceholder)}
help={intl.formatMessage(messages.createCollectionModalDescriptionDetails)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please send an element instead of a string so we don't have to change the types:

Suggested change
help={intl.formatMessage(messages.createCollectionModalDescriptionDetails)}
help={(
<Form.Text>
{intl.formatMessage(messages.createCollectionModalDescriptionDetails)}
</Form.Text>
)}

Comment on lines 38 to 42
createCollectionModalNameConflict: {
id: 'course-authoring.library-authoring.modals.create-collection.form.name.conflict',
defaultMessage: 'There is another collection with the same name',
description: 'Message when the Name field of the Create Collection modal form is not unique',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This message isn't used anywhere:

Suggested change
createCollectionModalNameConflict: {
id: 'course-authoring.library-authoring.modals.create-collection.form.name.conflict',
defaultMessage: 'There is another collection with the same name',
description: 'Message when the Name field of the Create Collection modal form is not unique',
},

src/library-authoring/data/apiHooks.ts Outdated Show resolved Hide resolved
@ChrisChV
Copy link
Contributor Author

@bradenmacdonald I need a green check from a reviewer with write access. Could you review this?

@bradenmacdonald
Copy link
Contributor

@ChrisChV, sure. Just updating my devstack now.

I did already notice one minor issue: when I click this "Add Collection" button, it doesn't open the "New Collection" modal - it just shows the general "Add" sidebar, which is not what I'd expect.

Screenshot 2024-09-13 at 10 23 17 AM

@bradenmacdonald
Copy link
Contributor

@ChrisChV Also, the following console warning appears when the modal is first opened, because there is still a problem with the blank help property.

Screenshot of a warning about empty FormText

➡️ I have a fix for you here: 0546f23

Copy link
Contributor

@bradenmacdonald bradenmacdonald 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! Conditional approval if you fix the above two issues (empty state button and console warning about help prop). Once you fix those and get a green build, go ahead and squash+merge :)

@ChrisChV ChrisChV merged commit a37a1b1 into openedx:master Sep 14, 2024
7 checks passed
@ChrisChV ChrisChV deleted the chris/FAL-3788-create-collection branch September 14, 2024 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FC Relates to an Axim Funded Contribution project open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants