feat: Pages and Resources components [BD-38] [TNL-8101] [BB-3972] #63

xitij2000 · 2021-03-30T17:00:02Z

TNL-8101 / TNL-8450
Hooks up the course apps API so that the data returned by the server is being used.
Adds the base components and infrastructure to enable adding pages for configuring each app.

This PR adds the foundation to the UI for adding settings components for individual pages in future PRs.

course-apps-calculator.mp4

openedx-webhooks · 2021-03-30T17:00:12Z

Thanks for the pull request, @xitij2000! I've created BLENDED-810 to keep track of it in Jira.

When this pull request is ready, tag your edX technical lead.

src/pages-and-resources/pages/PageGrid.jsx

src/pages-and-resources/pages/PageCard.jsx

src/pages-and-resources/app-settings-modal/AppSettingsModal.jsx

davidjoy · 2021-03-30T19:47:43Z

(tweaked the PR title to use conventional commits, as per the new OEP: https://github.com/edx/open-edx-proposals/blob/master/oeps/oep-0051-bp-conventional-commits.rst)

xitij2000 · 2021-03-30T19:53:53Z

@davidjoy This is stil WIP, and I plan to squash the commits with a proper message before this is ready.

src/generic/model-store/slice.js

src/index.jsx

src/pages-and-resources/pages/PageCard.jsx

src/pages-and-resources/pages/PageGrid.jsx

src/generic/enabled-disabled-badge/EnabledDisabledBadge.jsx

src/index.scss

asadazam93 · 2021-06-02T16:07:59Z

@xitij2000 Can you also add screenshots of how the pages look like?

src/pages-and-resources/PagesAndResources.jsx

mehaknasir

Some Logical and Console Errors are there which may be because this work is in progress. other things we have reviewed and have listed our findings for your reference.

src/pages-and-resources/teams/Settings.jsx

src/pages-and-resources/app-settings-modal/AppSettingsModal.jsx

mehaknasir · 2021-06-03T08:06:19Z

src/pages-and-resources/app-settings-modal/AppSettingsModal.jsx

+      >
+        <h3>{title}</h3>
+        <AppConfigFormDivider />
+        <Form onSubmit={onApply}>


why are not we using the Formik library for form management? by convention we are using Formik forms in codebase.

This PR started before that became convention :-)

I'll update this code to use that.

Actually I think in this case Formik might not work as well since the the base form doesn't know about the form elements injected in the child component.

We can use Formik in AppSettingsModal I see no reason why we can’t.

The structure of form does not look good to me and the modal is not closing on submit.
The type=submit will also work if you place this button inside form closing tag. I think this needs to be revisited.

@awaisdar001

We can use Formik in AppSettingsModal I see no reason why we can’t.
The reason using Formik here is harder is that the form is crated in AppSettingsModal, but the contents of the form will be created elsewhere. So Formik here will not know what all form elements the child components have added.

The fact that we can't use it here, doesn't mean we can't use it at all. We can use it in individual components, so I'll update those and share.

The structure of form does not look good to me and the modal is not closing on submit.
Could you highlight what doesn't look about the structure of the form?

The modal is not closing on submit on purpose for a couple of reasons:

The apply operation is a remote operation, so we'd want to convey the saving state, possibly via a stateful apply button, or some other UI element. So the form shouldn't close till the data is saved and we've confirmed to the use that the data is saved.

If the saving fails, and the form closes, it could be irritating for users so have to reopen the page, apply all the changes again (it could be a lot of teamsets) and then try saving again.

A user might want to make a couple of changes, apply them, and continue editing.

The type=submit will also work if you place this button inside form closing tag. I think this needs to be revisited.

I'll see if I can do something to restructure this to allow that to work.

src/pages-and-resources/teams/Settings.jsx

src/pages-and-resources/pages/PageCard.jsx

src/pages-and-resources/data/slice.js

src/pages-and-resources/notes/messages.js

mehaknasir · 2021-06-03T08:31:10Z

src/pages-and-resources/teams/TeamSetEditor.jsx

+        {isDeleting
+          ? (
+            <div className="d-flex flex-column m-4" key="isDeleting">
+              <h3>{intl.formatMessage(messages.team_set_delete_heading)}</h3>
+              <p>{intl.formatMessage(messages.team_set_delete_body)}</p>
+              <div className="d-flex flex-row justify-content-end">
+                <Button variant="muted" size="sm" onClick={() => onDelete(teamSet)}>
+                  {intl.formatMessage(messages.delete)}
+                </Button>
+                <Button variant="muted" size="sm" onClick={cancelDeletion}>
+                  {intl.formatMessage(messages.cancel)}
+                </Button>
+              </div>
+            </div>
+          )


Suggestion: this should be in generics to be used in other components as well. this is already in use at other places in the application.

I'm not sure what exactly you're referring to here?

codecov-commenter · 2021-06-24T14:28:29Z

Codecov Report

Merging #63 (5ad7f43) into master (44d8cfb) will decrease coverage by 4.75%.
The diff coverage is 2.97%.

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
- Coverage   73.31%   68.56%   -4.76%     
==========================================
  Files          67       75       +8     
  Lines        1023     1091      +68     
  Branches      192      211      +19     
==========================================
- Hits          750      748       -2     
- Misses        268      338      +70     
  Partials        5        5

Impacted Files	Coverage Δ
src/generic/FormSwitchGroup.jsx	`100.00% <ø> (ø)`
src/generic/status-badge/StatusBadge.jsx	`0.00% <0.00%> (ø)`
src/generic/status-badge/messages.js	`0.00% <0.00%> (ø)`
src/index.jsx	`0.00% <0.00%> (ø)`
src/pages-and-resources/PagesAndResources.jsx	`0.00% <0.00%> (ø)`
.../pages-and-resources/PagesAndResourcesProvider.jsx	`100.00% <ø> (ø)`
...-resources/app-settings-modal/AppSettingsModal.jsx	`0.00% <0.00%> (ø)`
...pages-and-resources/app-settings-modal/messages.js	`0.00% <0.00%> (ø)`
src/pages-and-resources/calculator/Settings.jsx	`0.00% <0.00%> (ø)`
src/pages-and-resources/calculator/messages.js	`0.00% <0.00%> (ø)`
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44d8cfb...5ad7f43. Read the comment docs.

xitij2000 · 2021-06-25T11:25:41Z

I've added a video to the PR description.

thanks - can you update update the description as well regarding it is expected that this PR is going to serve as a base for upcoming PRs? Thank you :)

Done.

src/index.jsx

src/pages-and-resources/PagesAndResources.jsx

mehaknasir · 2021-06-25T13:14:05Z

src/pages-and-resources/app-settings-modal/AppSettingsModal.jsx

+  return useFormik({
+    initialValues: {
+      enabled: !!appInfo?.enabled,
+      ...initialValues,
+    },
+    validationSchema: Yup.object()
+      .shape({
+        enabled: Yup.boolean(),
+        ...validationSchema,
+      }),
+    onSubmit: handleSubmit,
+  });


useFormik limits us sometimes we are restricted in many features e.g
Field, FastField, ErrorMessage, connect(), and FieldArray will NOT work with useFormik() as they all require React Context you can learn more here
Considering future perspective its best to use <Formik /> component instead which is more powerful.

As you can see in figma when you will need to work on team settings you will need to work with <FieldArray> then useFormik will not work. for help you can take help from this already implemented example

any thought on this one?

I will investigate this and update the PR accordingly.

I've updated the code to use Formik as you suggested.

src/pages-and-resources/calculator/Settings.jsx

mehaknasir

there are two pending things on this PR, I think if we can agree on some point on this we will be able to close it soon.

mehaknasir · 2021-06-28T17:18:02Z

src/pages-and-resources/pages/PageCard.jsx

-        <div>
-          {page.showStatus && <span>{intl.formatMessage(messages[pageStatusMsgId])}</span>}
+        <div className="my-1">
+          <StatusBadge status={page.enabled} />


I think the padding of the status badge is also needed to be fixed. The dividers used not part of the Figma design and are going out from both sides of the modal. you can see that in the attached video as well. if that is needed to be fixed in this PR. then you might need to look at it again.

mehaknasir · 2021-06-28T17:18:46Z

src/pages-and-resources/app-settings-modal/AppSettingsModal.jsx

+  return useFormik({
+    initialValues: {
+      enabled: !!appInfo?.enabled,
+      ...initialValues,
+    },
+    validationSchema: Yup.object()
+      .shape({
+        enabled: Yup.boolean(),
+        ...validationSchema,
+      }),
+    onSubmit: handleSubmit,
+  });


any thought on this one?

src/pages-and-resources/PagesAndResources.jsx

src/pages-and-resources/data/api.js

src/pages-and-resources/app-settings-modal/AppSettingsModal.jsx

awaisdar001 · 2021-06-30T09:30:16Z

src/pages-and-resources/data/thunks.js

+
+export function enableDisableApp(courseId, appId, state, syncChange = false) {
+  return async (dispatch) => {
+    if (!syncChange) {


Why this piece of if condition needed here? When we only need to update store and not make an API call? Can you elaborate?

Yes, it's used to persist state to the store without changing the state on the server.

Whats' the use case for this?

Hmm, I think it was used in a previous version of the code that got refactored out.

But generally the idea was, that if a user is applying complex changes to something like teams, you keep all those changes in the store, and save then so if the page accidentally closes, you don't lose those changes. You can then pick up where you left off and sync.

awaisdar001 · 2021-06-30T09:32:09Z

src/pages-and-resources/pages/PageCard.jsx

-  showStatus: PropTypes.bool.isRequired,
-  showEnable: PropTypes.bool.isRequired,
+  enabled: PropTypes.bool.isRequired,
+  allowed_operations: PropTypes.shape({


this should be =>allowedOperations

awaisdar001 · 2021-06-30T09:59:18Z

src/pages-and-resources/app-settings-modal/AppSettingsModal.jsx

+                <h3>{title}</h3>
+                <AppConfigFormDivider />
+                <FormSwitchGroup
+                  id={`enable-${appId}-toggle`}


Just checking why this ID is needed? The general perception for addingids is to things if we need to use the ID somewhere e.g. tests or referencing. I can't see any usage of this.

It is required by the FormSwitchGroup so I added it. I will update the component to make it optional.

I see, thank you.

Actually, reading through the comments in FormSwitchGroup it seems that currently you need to attach a label to the switch component.

awaisdar001

The changes looks good now, thanks for addressing them @xitij2000 :)

mehaknasir · 2021-07-01T08:00:20Z

This PR looks good to me. can you please rebase your commits in 1 commit and insert a commit message following commit conventions. Also, Let me know when you need to merge it. I will do that.

…ration Hooks up the course apps API so that the data returned by the server is being used. Adds the base components and infrastructure to enable adding pages for configuring each app.

xitij2000 · 2021-07-01T11:57:04Z

I've squashed down to a single commit.
@mehaknasir I don't have commit/merge access so you will need to merge for me :-)

awaisdar001 · 2021-07-01T15:58:14Z

Pr merged.

xitij2000 · 2021-07-02T05:40:34Z

@awaisdar001 Thanks!

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Mar 30, 2021

davidjoy reviewed Mar 30, 2021

View reviewed changes

src/pages-and-resources/pages/PageGrid.jsx Outdated Show resolved Hide resolved

davidjoy reviewed Mar 30, 2021

View reviewed changes

src/pages-and-resources/pages/PageCard.jsx Outdated Show resolved Hide resolved

davidjoy reviewed Mar 30, 2021

View reviewed changes

src/pages-and-resources/app-settings-modal/AppSettingsModal.jsx Outdated Show resolved Hide resolved

davidjoy changed the title ~~Pages and Resources components~~ feat: Pages and Resources components Mar 30, 2021

xitij2000 changed the title ~~feat: Pages and Resources components~~ feat: Pages and Resources components [BD-03] Mar 31, 2021

openedx-webhooks added blended PR is managed through 2U's blended developmnt program and removed open-source-contribution PR author is not from Axim or 2U labels Mar 31, 2021

xitij2000 commented Mar 31, 2021

View reviewed changes

src/generic/model-store/slice.js Outdated Show resolved Hide resolved

xitij2000 commented Mar 31, 2021

View reviewed changes

src/index.jsx Outdated Show resolved Hide resolved

xitij2000 commented Mar 31, 2021

View reviewed changes

src/pages-and-resources/pages/PageCard.jsx Outdated Show resolved Hide resolved

xitij2000 commented Mar 31, 2021

View reviewed changes

src/pages-and-resources/pages/PageGrid.jsx Show resolved Hide resolved

xitij2000 force-pushed the kshitij/pages-and-resources branch 2 times, most recently from e1bb64f to 42e637b Compare April 12, 2021 05:06

xitij2000 force-pushed the kshitij/pages-and-resources branch from 42e637b to 0518865 Compare May 25, 2021 10:57

asadazam93 reviewed Jun 2, 2021

View reviewed changes

src/generic/enabled-disabled-badge/EnabledDisabledBadge.jsx Outdated Show resolved Hide resolved

src/index.scss Outdated Show resolved Hide resolved

mehaknasir reviewed Jun 3, 2021

View reviewed changes

src/pages-and-resources/PagesAndResources.jsx Show resolved Hide resolved

mehaknasir reviewed Jun 3, 2021

View reviewed changes

src/pages-and-resources/PagesAndResources.jsx Show resolved Hide resolved

mehaknasir suggested changes Jun 3, 2021

View reviewed changes

xitij2000 changed the title ~~feat: Pages and Resources components [BD-03]~~ feat: Pages and Resources components [BD-38] Jun 10, 2021

xitij2000 force-pushed the kshitij/pages-and-resources branch 3 times, most recently from 91106ba to 3832f35 Compare June 24, 2021 14:26

xitij2000 marked this pull request as ready for review June 24, 2021 14:42

mehaknasir suggested changes Jun 25, 2021

View reviewed changes

xitij2000 changed the title ~~feat: Pages and Resources components [BD-38]~~ feat: Pages and Resources components [BD-38] [TNL-8101] Jun 28, 2021

mehaknasir suggested changes Jun 28, 2021

View reviewed changes

xitij2000 changed the title ~~feat: Pages and Resources components [BD-38] [TNL-8101]~~ feat: Pages and Resources components [BD-38] [TNL-8101] [BB-3972] Jun 28, 2021

xitij2000 force-pushed the kshitij/pages-and-resources branch 2 times, most recently from 22fe883 to ddc937f Compare June 30, 2021 04:08

awaisdar001 reviewed Jun 30, 2021

View reviewed changes

src/pages-and-resources/PagesAndResources.jsx Show resolved Hide resolved

awaisdar001 reviewed Jun 30, 2021

View reviewed changes

src/pages-and-resources/data/api.js Outdated Show resolved Hide resolved

awaisdar001 reviewed Jun 30, 2021

View reviewed changes

src/pages-and-resources/app-settings-modal/AppSettingsModal.jsx Outdated Show resolved Hide resolved

awaisdar001 reviewed Jun 30, 2021

View reviewed changes

src/pages-and-resources/app-settings-modal/AppSettingsModal.jsx Show resolved Hide resolved

awaisdar001 reviewed Jun 30, 2021

View reviewed changes

src/pages-and-resources/app-settings-modal/AppSettingsModal.jsx Show resolved Hide resolved

awaisdar001 reviewed Jun 30, 2021

View reviewed changes

src/pages-and-resources/app-settings-modal/AppSettingsModal.jsx Outdated Show resolved Hide resolved

awaisdar001 reviewed Jun 30, 2021

View reviewed changes

xitij2000 force-pushed the kshitij/pages-and-resources branch 2 times, most recently from 16e22a5 to b8da9e9 Compare July 1, 2021 05:13

awaisdar001 approved these changes Jul 1, 2021

View reviewed changes

mehaknasir approved these changes Jul 1, 2021

View reviewed changes

feat: Hook up course apps API and add base components for app configu…

5ad7f43

…ration Hooks up the course apps API so that the data returned by the server is being used. Adds the base components and infrastructure to enable adding pages for configuring each app.

xitij2000 force-pushed the kshitij/pages-and-resources branch from b8da9e9 to 5ad7f43 Compare July 1, 2021 11:55

awaisdar001 merged commit 0e7340b into openedx:master Jul 1, 2021

openedx-webhooks added merged and removed needs triage labels Jul 1, 2021

xitij2000 deleted the kshitij/pages-and-resources branch July 2, 2021 05:40

feat: Pages and Resources components [BD-38] [TNL-8101] [BB-3972] #63

feat: Pages and Resources components [BD-38] [TNL-8101] [BB-3972] #63

Conversation

xitij2000 commented Mar 30, 2021 • edited by awaisdar001

openedx-webhooks commented Mar 30, 2021 • edited

davidjoy commented Mar 30, 2021

xitij2000 commented Mar 30, 2021

asadazam93 commented Jun 2, 2021

mehaknasir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 24, 2021 • edited

Codecov Report

xitij2000 commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehaknasir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaisdar001 left a comment

Choose a reason for hiding this comment

mehaknasir commented Jul 1, 2021

xitij2000 commented Jul 1, 2021

awaisdar001 commented Jul 1, 2021

xitij2000 commented Jul 2, 2021

xitij2000 commented Mar 30, 2021 •

edited by awaisdar001

openedx-webhooks commented Mar 30, 2021 •

edited

codecov-commenter commented Jun 24, 2021 •

edited