updated material-ui and dialog responsiveness #153 #155
Conversation
| }] | ||
| } | ||
| } | ||
| "env": { |
There was a problem hiding this comment.
Are hooks being used anywhere right now?
There was a problem hiding this comment.
Hey Daniel, great work with this PR! Left a few initial comments here.
A few of my main concerns were:
- PR getting a bit big. Initially, my thought was that we could just modify a few CSS/Dialog component attributes which would default to fullScreen if the screen size was too big. (You could use the
MediaQuerycomponent used throughout the app). - Try not to update
package.jsonor add too many dependencies. I would first try to do it with only the components at hand. But, if that is not possible or if you have difficulties, just reach out to me and we can work through it together! - Is it possible to make a PR with just one component change? That way, it would be easier for me to code review and provide feedback. Once I approve that change, we can duplicate it across all other components. Does that sound good?
| "@material-ui/core": "^4.0.0-rc.0", | ||
| "@sentry/browser": "^5.7.1", | ||
| "@sentry/node": "^5.7.1", | ||
| "async": "^2.6.3", |
There was a problem hiding this comment.
Is it possible to leave the versions as they were? In case of any breaking changes to other parts of the code.
| @@ -7,16 +7,20 @@ | |||
| "redux-logger": "^3.0.6" | |||
| }, | |||
| "dependencies": { | |||
| "@material-ui/core": "^3.3.2", | |||
| "@material-ui/core": "^4.0.0-rc.0", | |||
There was a problem hiding this comment.
Let's try to use the same version of material ui. They're constantly updating, so it's a bit difficult to keep up.
| "classnames": "^2.2.6", | ||
| "cross-env": "^5.2.0", | ||
| "cryptiles": "^4.1.2", | ||
| "json2mq": "^0.2.0", |
| "material-ui-search-bar": "^0.4.2", | ||
| "rc-progress": "^2.2.6", | ||
| "react-beautiful-dnd": "^9.0.2", | ||
| "react-dimensions": "^1.3.1", | ||
| "react-facebook-login": "^4.1.1", | ||
| "react-full-screen": "^0.2.4", | ||
| "react-screen-orientation": "0.0.4", |
There was a problem hiding this comment.
Are all these added packages necessary? Just wondering because I would prefer to introduce as few new packages as possible to prevent the size of our dependencies from growing too large.
| autoScrollBodyContent | ||
| fullWidth={ true } |
There was a problem hiding this comment.
Good work! This was basically the kind of change I thought this PR would need, just minimal CSS changes and component attribute modifications.
| @@ -335,7 +313,21 @@ export default class TermBoard extends Component { | |||
| : ( | |||
| <div> | |||
| <MenuItem primaryText="Edit Name" onClick={ this.openRenameDialog } /> | |||
| <MenuItem primaryText="Import Courses" onClick={ this.openImportDialog } /> | |||
|
|
|||
| {/* If screen of device is 'sm' and in portrait mode, then Dialog Import Courses wont open, | |||
There was a problem hiding this comment.
So, if the device screen is small and in portrait mode, it will default to landscape mode?
| ${watchlist[toBeDeleted].catalogNumber} (${watchlist[toBeDeleted].classNum}) | ||
| from your watchlist? | ||
| ` } | ||
| <DialogTitle id="responsive-dialog-title">{"Are you sure?"}</DialogTitle> |
There was a problem hiding this comment.
Why do we need the id of responsive-dialog-title?
Description
Updated material-ui version to newer for dialog responsiveness, also added rules for future use of hooks for eslint. Added dependencies related with dialogs: material-ui-fullscreen-dialog, react-full-screen, react-screen-orientation
Non-responsive dialongs #153
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Running web app at multiple devices to check responsiveness.
Checklist: