search

openedx / frontend-app-gradebook

Instructor grade book tool
GNU Affero General Public License v3.0
11 stars 87 forks source link

refactor: reduce scss custom variables and classes #258

Closed navinkarkera closed 1 year ago

navinkarkera commented 2 years ago

TL;DR - Removes unnecessary custom scss classes and paragon variables and replaces with bootstrap classes.

What changed?

Screenshots

Main screen before: before-main-screen Main screen after: (Only change is use of max-height instead of height for table) after-main-screen

Main screen drawer before: before-main-screen-open-drawer Main screen drawer after: after-main-screen-open-drawer

Bulk management before: before-bulk-screen Bulk management after: after-bulk-screen

Modal before: before-modal Modal after:

Here the Key value display is updated to use bootstrap grid instead of hacky spacing. after-modal Let me know if this looks acceptable

Developer Checklist

Testing Instructions

Compare the look and feel of application before and after this PR.

Reviewer Checklist

Collectively, these should be completed by reviewers of this PR:

FYI: @openedx/content-aurora @edx/educator-neem

openedx-webhooks commented 2 years ago

Thanks for the pull request, @navinkarkera! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

natabene commented 2 years ago

@navinkarkera Thank you for your contribution. Let's see how tests go.

codecov[bot] commented 2 years ago

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage :thumbsup:

Coverage data is based on head (ddfe8e0) compared to base (47cab71). Patch coverage: 100.00% of modified lines in pull request are covered.

:exclamation: Current head ddfe8e0 differs from pull request most recent head 9b9ccf8. Consider uploading reports for the commit 9b9ccf8 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #258 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 108 108 Lines 1259 1259 Branches 247 247 ========================================= Hits 1259 1259 ``` | [Impacted Files](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/258?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx) | Coverage Δ | | |---|---|---| | [...ponents/BulkManagementHistoryView/HistoryTable.jsx](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/258/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2NvbXBvbmVudHMvQnVsa01hbmFnZW1lbnRIaXN0b3J5Vmlldy9IaXN0b3J5VGFibGUuanN4) | `100.00% <ø> (ø)` | | | [src/components/BulkManagementHistoryView/index.jsx](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/258/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2NvbXBvbmVudHMvQnVsa01hbmFnZW1lbnRIaXN0b3J5Vmlldy9pbmRleC5qc3g=) | `100.00% <ø> (ø)` | | | [...onents/GradebookFilters/AssignmentFilter/index.jsx](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/258/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2NvbXBvbmVudHMvR3JhZGVib29rRmlsdGVycy9Bc3NpZ25tZW50RmlsdGVyL2luZGV4LmpzeA==) | `100.00% <ø> (ø)` | | | [...s/GradebookFilters/AssignmentGradeFilter/index.jsx](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/258/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2NvbXBvbmVudHMvR3JhZGVib29rRmlsdGVycy9Bc3NpZ25tZW50R3JhZGVGaWx0ZXIvaW5kZXguanN4) | `100.00% <ø> (ø)` | | | [...ts/GradebookFilters/AssignmentTypeFilter/index.jsx](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/258/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2NvbXBvbmVudHMvR3JhZGVib29rRmlsdGVycy9Bc3NpZ25tZW50VHlwZUZpbHRlci9pbmRleC5qc3g=) | `100.00% <ø> (ø)` | | | [...nents/GradebookFilters/CourseGradeFilter/index.jsx](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/258/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2NvbXBvbmVudHMvR3JhZGVib29rRmlsdGVycy9Db3Vyc2VHcmFkZUZpbHRlci9pbmRleC5qc3g=) | `100.00% <ø> (ø)` | | | [src/components/GradebookFilters/index.jsx](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/258/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2NvbXBvbmVudHMvR3JhZGVib29rRmlsdGVycy9pbmRleC5qc3g=) | `100.00% <ø> (ø)` | | | [...c/components/GradesView/EditModal/ModalHeaders.jsx](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/258/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2NvbXBvbmVudHMvR3JhZGVzVmlldy9FZGl0TW9kYWwvTW9kYWxIZWFkZXJzLmpzeA==) | `100.00% <ø> (ø)` | | | [src/components/GradesView/GradebookTable/index.jsx](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/258/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2NvbXBvbmVudHMvR3JhZGVzVmlldy9HcmFkZWJvb2tUYWJsZS9pbmRleC5qc3g=) | `100.00% <ø> (ø)` | | | [src/components/GradesView/ImportGradesButton.jsx](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/258/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2NvbXBvbmVudHMvR3JhZGVzVmlldy9JbXBvcnRHcmFkZXNCdXR0b24uanN4) | `100.00% <ø> (ø)` | | | ... and [8 more](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/258/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx) | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

navinkarkera commented 2 years ago

@xitij2000 Updated the MR as per your comments as well as removed additional change like modal key-value display style as posted in the last screenshot of description.

those moving from px measurements to rem.

So should we revert all changes where we are using bootstrap's m- & p- instead of custom classes?

xitij2000 commented 2 years ago

@xitij2000 Updated the MR as per your comments as well as removed additional change like modal key-value display style as posted in the last screenshot of description.

those moving from px measurements to rem.

So should we revert all changes where we are using bootstrap's m- & p- instead of custom classes?

My concern is that this PR is changing too much and will be hard to review if too many things change at once. Switching from something like 20px to 1.5rem might make very little visual difference, but I'm not sure why the 20px was picked in the first place. It may have been for a reason I don't know.

Also, the main aim here is to get rid of variable usage, and cleaning up other scss is a lot less important in the context of this work.

navinkarkera commented 2 years ago

@xitij2000

the main aim here is to get rid of variable usage

Would it make more sense to create a separate MR with just removing variable usage, although we would just have one line change i.e. $black variable usage in GradesView.scss file.

And we could leave this MR and review whenever feasible?

xitij2000 commented 2 years ago

@xitij2000

the main aim here is to get rid of variable usage

Would it make more sense to create a separate MR with just removing variable usage, although we would just have one line change i.e. $black variable usage in GradesView.scss file.

And we could leave this MR and review whenever feasible?

Yes, let's do that.

navinkarkera commented 2 years ago

@xitij2000 Created #259

schenedx commented 2 years ago

The problem I have with this PR is that, the drop shadow at the bottom of the main grades table disappeared. Can that be added back?

xitij2000 commented 2 years ago

I believe the change here makes sense in general. @navinkarkera can you speak to what triggered this change for you? I haven't had the time to setup my local copy to reference your forked branch yet. I will do that next week, so I can test this change out on my local machine.

I can answer that a bit.

OpenCraft's been working on some theming improvements, and one of those improvements is to make the UI more responsive to theming, which means more reliance on common classes that can be overridden vs custom CSS. Another aspect is to make it possible to compile an MFE's SCS independently of paragon. This will allow compiling the common paragon + theme and potentially loading the same SCSS in all MFEs. For this it's important for MFEs to not use any bootstrap or paragon variables in SASS.

Using CSS variables means that we can have a common theme that changes and that affects all MFEs instead of needing to recompile each one.

navinkarkera commented 2 years ago

@navinkarkera can you speak to what triggered this change for you?

@schenedx Because we are already using bootstrap which covers most of the requirements of these custom classes.

I haven't had the time to setup my local copy to reference your forked branch yet. I will do that next week, so I can test this change out on my local machine.

Thank you and sorry, I did not have permissions to create/push new branch on this repo, hence the fork.

The problem I have with this PR is that, the drop shadow at the bottom of the main grades table disappeared. Can that be added back?

Actually, the drop shadow is part of datatable and it only used to appear if we have rows with total height less than 600px. I have now added shadow to the container making it always visible.

Rows < 600px: image

Rows > 600px: image

schenedx commented 2 years ago

@xitij2000 Do you have any going forward plan for this PR? Should we make further updates to align with what 2U internal frontend working group wants to do for the long term?

xitij2000 commented 2 years ago

@xitij2000 Do you have any going forward plan for this PR? Should we make further updates to align with what 2U internal frontend working group wants to do for the long term?

I wanted to get css variables into Paragon first so I could replace some of the harder-to-replace variables in this PR. However, I can also just limit the scope here and leave that for a future PR.

schenedx commented 2 years ago

@xitij2000 If you made a decision on which way you want to go, could you either link the Paragon PR here, or let us know that the revised PR here is ready for another review? Thank you!

navinkarkera commented 2 years ago

@xitij2000 Do you have any going forward plan for this PR? Should we make further updates to align with what 2U internal frontend working group wants to do for the long term?

I wanted to get css variables into Paragon first so I could replace some of the harder-to-replace variables in this PR. However, I can also just limit the scope here and leave that for a future PR.

@xitij2000 Let me know the plan forward. Will #259 be good enough for now?

schenedx commented 1 year ago

@xitij2000 Could you express your opinions on this PR? Is it too out of date by now and we should close?

xitij2000 commented 1 year ago

@xitij2000 Could you express your opinions on this PR? Is it too out of date by now and we should close?

@schenedx I think the main goal here was get rid of paragon variables, and the secondary goal was to reduce custom scss in general in favour of pre-existing classes.

I think the first of those is just a one line change, and the rest is more extensive than I would have liked in one go.

@navinkarkera Could you look at some of the comments in this PR and perhaps split out the single variable change into a separate PR that can be merged independently?

navinkarkera commented 1 year ago

@navinkarkera Could you look at some of the comments in this PR and perhaps split out the single variable change into a separate PR that can be merged independently?

@xitij2000 I had already created https://github.com/openedx/frontend-app-gradebook/pull/259 for the same reason and posted here. It was closed recently, I might be missing something here as it has been a long time since I created this MR.

mphilbrick211 commented 1 year ago

Hi @xitij2000 and @navinkarkera - just following up on this.

mphilbrick211 commented 1 year ago

@navinkarkera @xitij2000 - one more friendly ping on this :)

mphilbrick211 commented 1 year ago

Hi @navinkarkera - any update on this?

navinkarkera commented 1 year ago

@mphilbrick211 Apologies for delay.

@xitij2000 I had already created https://github.com/openedx/frontend-app-gradebook/pull/259 for the same reason and posted https://github.com/openedx/frontend-app-gradebook/pull/258#issuecomment-1236846237. It was closed recently, I might be missing something here as it has been a long time since I created this MR.

@xitij2000 Gentle reminder.

xitij2000 commented 1 year ago

Hi @navinkarkera - any update on this?

Hi, sorry for the delay. I've mostly been waiting for the design tokens work to land before looking into these PRs again, since that work will inform how we handle this.

I think in the case of this PR, we have very limited budget to pursue it further but @navinkarkera if you feel you could remove some of the controversial changes perhaps we can get it closer to merging? I don't feel like most of this work will be affected by design tokens.

@navinkarkera If you feel like there is significant work left here to get this up to date and mergeable do tell and I'll see what we can do.

schenedx commented 1 year ago

@navinkarkera and @xitij2000 I think the longer this is left out in the open, the more work to bring this PR up-to-date. Therefore my suggestion is to close the PR. And when you are ready, create a new PR, on existing branch or new branch, and we will review again. Would you agree with that?

mphilbrick211 commented 1 year ago

@navinkarkera and @xitij2000 I think the longer this is left out in the open, the more work to bring this PR up-to-date. Therefore my suggestion is to close the PR. And when you are ready, create a new PR, on existing branch or new branch, and we will review again. Would you agree with that?

Hi @navinkarkera and @xitij2000 - following up on @schenedx's comment above. Please let us know if this is OK to close. Thanks!

xitij2000 commented 1 year ago

@navinkarkera and @xitij2000 I think the longer this is left out in the open, the more work to bring this PR up-to-date. Therefore my suggestion is to close the PR. And when you are ready, create a new PR, on existing branch or new branch, and we will review again. Would you agree with that?

Hi @navinkarkera and @xitij2000 - following up on @schenedx's comment above. Please let us know if this is OK to close. Thanks!

Sorry, Yeah I don't seem to be getting notifications like I used to. Let's close this for now and we'll resume and crate a new PR if needed.

mphilbrick211 commented 1 year ago

Closing per comments above.

openedx-webhooks commented 1 year ago

@navinkarkera Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.