mitodl / edx-platform

The Open edX platform, the software that powers edX!
http://open.edx.org/
GNU Affero General Public License v3.0
6 stars 1 forks source link

Maple: Gradebook MFE #277

Closed pdpinch closed 2 years ago

pdpinch commented 2 years ago

Like the Learner MFE, the Gradebook MFE needs to be updated to conform with the current approach to headers and footers.

We need to refactor frontend-app-gradebook so that it uses frontend-component-header and frontend-component-footer instead of it's own built-in header and footer.

We may also need to make updates to frontend-component-header-edx and frontend-component-footer-edx before edX can accept the changes to frontend-app-gradebook.

asadiqbal08 commented 2 years ago

@pdpinch here comes few questions to ask. 1- Do you mean to update the frontend-app-gradebook to use the Header OR LearningHeader component from frontend-component-header instead of using its own GradebookHeader ? 2- Are you thinking to remove the GradebookHeader component from the frontend-app-gradebook code base or keep that component there ? 3- I can see the frontend-app-gradebook is already utilising the footer from frontend-component-footer inside this file app.jsx, Do you mean to point to some other part in repo ? 4 -

We may also need to make updates to frontend-component-header-edx and frontend-component-footer-edx before edX can accept the changes to frontend-app-gradebook.

Sorry I did not get this point, Do you mean, we need to move the GradebookHeader component from this repo to frontend-component-header and frontend-component-header-edx and then use that header from there ? If that is the point then I get the answer of my point#1 and point#2 above.

5- Right now, I am getting this UI on the screen. Am I on the right page of UI in terms of styling ? screencapture-localhost-1994-course-v1-edX-DemoX-Demo-Course-2021-12-09-17_11_22

pdpinch commented 2 years ago

I opened this issue in response to https://github.com/openedx/build-test-release-wg/issues/29#issuecomment-950935441

1- Do you mean to update the frontend-app-gradebook to use the Header OR LearningHeader component from frontend-component-header instead of using its own GradebookHeader ?

I'm sorry. I don't know the different between Header and LearningHeader which do you think is a better replacement for GradebookHeader ?

2- Are you thinking to remove the GradebookHeader component from the frontend-app-gradebook code base or keep that component there ?

Yes, if we can replace GradebookHeader with a header imported from frontend-component-header, then I think we should remove the GradebookHeader component from the frontend-app-gradebook

3- I can see the frontend-app-gradebook is already utilising the footer from frontend-component-footer inside this file app.jsx, Do you mean to point to some other part in repo ?

I think this is the correct behavior. I saw that @jfavellar90 was having some difficulty with the footer, so I wanted to be sure it was working correctly.

4 - Do you mean, we need to move the GradebookHeader component from this repo to frontend-component-header and frontend-component-header-edx and then use that header from there ? If that is the point then I get the answer of my point#1 and point#2 above.

I'm not sure. What do you think? Some things to consider:

  1. For future developers, it would be easier to understand the code if Learning and Gradebook MFEs followed a similar pattern.
  2. What will make it easier for us to customize the header, if we need to (for example, adding or removing items from the user menu)
  3. What does David (and the frontend working group) think?

5- Right now, I am getting this UI on the screen. Am I on the right page of UI in terms of styling ?

Looks good to me.

davidjoy commented 2 years ago

I'd suggest that the gradebook MFE, being instructor-facing, would use a different header than the learning MFE. So yeah, we should move its header into the shared header repo, but in spirit we don't want 1-to-1 headers in frontend-component-header for each MFE... we want them for different domains, like learning, studio, marketing, etc.

Point being, the name probably shouldn't be GradebookHeader but something more general, as we might expect it to be reused for other instructor-facing MFEs.

jfavellar90 commented 2 years ago

@pdpinch @davidjoy @asadiqbal08 I'm working on a PR to enable the regular frontend-component-header in the gradebook MFE. So far it looks like this:

Screenshot from 2021-12-10 09-53-20

It requires a bit of styling, however I think the regular header would be OK for this MFE, Should it have a different default, like a courseware header (shared by Learning and Gradebook MFE)? Anyways you can always override it. A couple of tests are failing after my changes, however I'll post my PR in the next few hours so you can take a look.

I totally agree with Peter: For future developers, it would be easier to understand the code if Learning and Gradebook MFEs followed a similar pattern. It also applies for the tooling used, for instance in the Account and Learning MFE's I see there's a bot that updates the dependencies of these projects regularly, this feature is not present in the Gradebook MFE which miss the latest versions of the Header, Footer and other components.

Regarding my difficulty with the footer override, it is related to the absence of the "dist" folder in my Github repo. The reason why it works for frontend-component-header and frontend-component-footer default components is the fact that those packages were uploaded to an NPM registry with the "dist" folder included. Once I added that folder to my repo (https://github.com/eduNEXT/frontend-component-footer/commit/89015dc1fec7da89313b0989181bc7463216c7ba), the override worked as expected. Is it OK to do so? Please let me know your thoughts on this one.

I remain attentive to any additional feedback.

pdpinch commented 2 years ago

Let's put this on hold to see how things have progressed upstream.

pdpinch commented 2 years ago

These seems to be working well for us on courses-qa.mitxonline.

If we need to backport this to maple, it should be an issue in a different project, like xPRO or residential.