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

I1529 Frontend Dependancy Update #1555

Merged
merged 19 commits into from
Jan 10, 2024

Conversation

pushyamig
Copy link
Contributor

@pushyamig pushyamig commented Dec 7, 2023

Fixes #1529 and #1525

  1. Fix the UI changes due to MUI v5 update as it seems to be due to MUI adopting https://emotion.sh/docs/introduction
    1. https://mui.com/material-ui/migration/migration-v4/. The documentation is nuanced since it talk all scenarios with upgrade to MUi v5 with React 17, MUi v5 with React 18, MUi v5 supporting JSS etc
    2. used CodeMods for updating the MUI to v5. here are the docs what gets updated https://github.com/mui/material-ui/blob/master/packages/mui-codemod/README.md#variant-prop
  2. react-router-dom has some major changes, the nested router path are relative now
    1. withRouter higher order hook is removed, so need to write a custom function
  3. React 18 had some changes to how react is initiated. lot of Improvement to async support.
  4. The strategy for package-lock.json was just delete node_module and update the package.json and generate a new lock file. This was simplest!
  5. upgrade to @apollo/client, Assignment planning was crashing so added a configuration option assumeImmutableResults: false, to enable mutable object of response which was allowed as an configuration option.
  6. used npm-check-updates for checking the latest package version in the repo. It is recommended by package dev to install it globally. https://www.npmjs.com/package/npm-check-updates npm install -g npm-check-updates
  7. Three security advisory w.r.t (postcss, graphql, @babel/traverse) are resolved now.
    check using the command npm ls <package-name> Eg., npm ls postcss

Todo List:

  • UI changes from MUI v5 upgrade ( There might be more just need to dig down) - Jaydon
    • Landing page layout and View card layout
      • disable button is not rendering color grey when the view is disabled and on hover
      • make sure to add Copy right flatpage (via django admin tool). I don't see anychanges needed here
    • Resources access view tiny form
    • Resource accessed Drop-down box is slight touching the slider. Needs some adjustment
    • Making the bar %value visible with student accessed the file
    • Assignment planning Pop-up window: no styling applied
      • Assignment Planning pop-up: exiting mouse hover doesn't close the popover
    • Survey Model is not centered after clicking the survey button.
      • Survey Model when opening up causing an warning in console log
    • On hover of the survey button the the color contrast making it Opaque
      Reproduce it Admin -> Config -> SURVEY_URL
    • the Listitem component button attribute seems deprecated ( it is stricked out in VsCode). Needs an understanding if any impact and any otherway to compensate it. Documentation will help
    • Need an understanding in impact during the upgrade import IconButton from '@mui/material/IconButton' the iconButton Component added size='large' to all files
    • use of sx prop. MUI v5 introduced this prop for inline styling for certain component. for older version style prop was used. I don't feel we need to replace style with sx but building some understanding will help. https://mui.com/system/getting-started/the-sx-prop/
      https://stackoverflow.com/questions/72527461/when-should-i-use-style-instead-of-sx-prop-in-material-ui#:~:text=sx%20prop%20works%20only%20on,some%20cases%20as%20explained%20below.
  • Webpack/babel lib update and build update - Pushyami
  • Other packages update react-router-dom, rc-slider, d3 etc - Pushyami
  • strategy around package-lock.json commit - Pushyami
  • Over all testing - Pushyami/Jaydon
  • Jest testing as things are not working as expected (Fix Jest Frontend tests #1558)

Test Plan

https://docs.google.com/spreadsheets/d/18c_VDE7wdme8c36Zo5-pjad77Kit8hFz4ylbmFXmLP8/edit#gid=0

Testing should be done from Student, instructor and MyLa Admin role

@pushyamig
Copy link
Contributor Author

This Draft PR is branched out of #1554 since that has major Backend updates. So it make sense to this way until that PR get merged

@pushyamig
Copy link
Contributor Author

used CodeMods for updating the MUI to v5. here are the docs what gets updated https://github.com/mui/material-ui/blob/master/packages/mui-codemod/README.md

/> }
/>
</Route>
<Route
Copy link
Contributor Author

Choose a reason for hiding this comment

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

react-router-dom v6 had new approach, nested routes became relative than absolute before.


export default withRouter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

V6 of the react-router-dom removed withRouter, so I have to write this HOC as per the documentation guide, linked in the page

return (
<>
<Helmet titleTemplate='%s | My Learning Analytics' title='Courses' />
<GoogleAnalyticsTracking {...{ gaId, cspNonce }} />
<Routes>
<Route path='/' element={<CourseList user={user} />} />
<Route path='/courses' element={ <CourseList user={user} />} />
<Route path='/courses/:courseId/*' element={courseId ? <Course user={user} courseId={Number(courseId)} {...props} /> : <AlertBanner>Application Launch did not happened properely</AlertBanner>} />
<Route path='/courses' element={<CourseList user={user} />} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

part of react-router-dom v6 change

? [0, 0.1]
: event.selection
? [0, 0.1]
: event.selection
: defaultSelection
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spacing issue

@@ -35,15 +35,17 @@ const rangeSlider = props => {
range
activeDotStyle={activeDotStyle}
dotStyle={dotStyle}
handleStyle={[activeDotStyle, activeDotStyle]}
railStyle={unselectedStyle}
trackStyle={[selectedStyle]}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are deprecated in the new version of rc-slider and includes as part of the styles property.

@@ -1,6 +1,6 @@
import React from 'react'
import 'rc-slider/assets/index.css'
import Slider, { Range } from 'rc-slider'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rc-slider package has some solid change, but not breaking. I felt the slider looked ok and functioned as before

<div key={key}>
<Line {...line} barHeight={height} outOf={outOf} />
</div>
))
: null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spacing change

@@ -1,6 +1,6 @@
import React, { useState } from 'react'
import { styled } from '@mui/material/styles'
import { useLocation } from 'react-router'
import { useLocation } from 'react-router-dom'
import Button from '@mui/material/Button'
Copy link
Contributor Author

@pushyamig pushyamig Dec 13, 2023

Choose a reason for hiding this comment

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

react-router package is package dependency and we never had in our package.json file. So I changed using the react-router-dom package. properties that are being used here also present in it https://reactrouter.com/en/main/hooks/use-location.

@@ -235,11 +235,11 @@ const SelectCard = props => {
</CardContent>
)

return <Root>{cardImage}{cardContent}</Root>
return <>{cardImage}{cardContent}</>
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is nice and simple fix. I pulled the latest and things are looking good with this commit.

@@ -92,7 +92,7 @@ export default function SurveyModal (props) {
const toggleOpen = () => setOpen(!open)

const body = (
<Root className={`${classes.paper} ${classes.modal}`}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaydonkrooss I couldn't test this, but I felt this would work. were you able to test this with survey link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The button looks good, But the modal after clicking is not centered. I added 2 more issue around this.

You can reproduced it locally by going to Admin -> Config -> SURVEY_URL "Give any URL"

@pushyamig
Copy link
Contributor Author

I have built the latest from this repo which is here https://github.com/pushyamig/my-learning-analytics/actions/runs/7413997544

@pushyamig pushyamig marked this pull request as ready for review January 4, 2024 20:05
@pushyamig
Copy link
Contributor Author

This PR is now ready for Review, I have added test plan as well

@pushyamig pushyamig self-assigned this Jan 4, 2024
@pushyamig
Copy link
Contributor Author

I pretty much tested/reviewed it from my lense and things are ok .

I have updated things I tested according to test plan
https://docs.google.com/spreadsheets/d/18c_VDE7wdme8c36Zo5-pjad77Kit8hFz4ylbmFXmLP8/edit#gid=0

@pushyamig pushyamig assigned pushyamig and unassigned pushyamig Jan 10, 2024
@pushyamig
Copy link
Contributor Author

I can't approve a PR which is opened by me. But I will approve the Jaydon's work as well

@pushyamig pushyamig merged commit dca2734 into tl-its-umich-edu:master Jan 10, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Frontend Dependencies
2 participants