Skip to content
This repository was archived by the owner on Oct 7, 2020. It is now read-only.

Conversation

cindyunrau
Copy link

Added Current and Previous Courses to the drawer:
To-do:

  • fix issue where all nested lists open whenever one is clicked
  • GPA is currently hard-coded
  • courses need to be taken and sorted into the appropriate semesters based on date range

Copy link
Contributor

@BraidenCutforth BraidenCutforth left a comment

Choose a reason for hiding this comment

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

Looks really good! Just need to make some changes in drawer content mainly and I think it will be ready to go!

@@ -0,0 +1,6 @@
import React from 'react'
Copy link
Contributor

Choose a reason for hiding this comment

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

Test component?

Copy link
Author

Choose a reason for hiding this comment

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

I was going to use this to take the input date range from courses and sort them into their appropriate semesters. Didn't start implementation yet.

@cindyunrau cindyunrau marked this pull request as ready for review November 28, 2019 19:55
Copy link
Contributor

@BraidenCutforth BraidenCutforth left a comment

Choose a reason for hiding this comment

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

Looks good! We should look into trying to use the material-ui styles though as suggested. Once we make that change we are good to merge!

Comment on lines 28 to 33
const drawerContent: CSS.Properties = {
backgroundColor: '#F5F5F5',
paddingTop: '30px',
paddingBottom: '30px',
height: '100%',
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use the material-ui styling ways instead of CSS properties. We can convert this pretty easily. In the useStyles const, add a key like

drawerContent: {
    backgroundColor: '#F5F5F5', // There might be something like theme.pallette.something, if you can find it, great, if not just use this constant for now
    paddingTop: theme.spacing(1), // 1 or more, adjust based on what you want
    paddingBottom: theme.spacing(1), // same as above
    height: '100%',
}

Once that is done, then you can use the className property instead of the styles property, suggested below.

paddingBottom: '30px',
height: '100%',
}
const textStyles: CSS.Properties = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar as above.

// On any nested loop click, all nested lists open and close -> should only open and close specific nested list

return (
<div id="drawer_content" style={drawerContent}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Once you change the way the styling is written above, this is the change you need down here

Suggested change
<div id="drawer_content" style={drawerContent}>
<div id="drawer_content" className={classes.drawerContent}>

"react": "^16.11.0",
"react-dom": "^16.11.0"
}
"name": "passr",
Copy link

@daelynj daelynj Dec 10, 2019

Choose a reason for hiding this comment

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

@BraidenCutforth It looks like your editor applied some formatting to package.json and package-lock.json by accident. This is avoidable if you only add the files to your commits that you've worked on rather than doing a git add . to commit every change.

This has small effects such as changing 7587 lines of code in the repository. Also changing the history of the two files, and when this code gets merged, will change everyone's versions of these files as well. But it's solvable with an interactive rebase if you feel like it!

Copy link
Contributor

Choose a reason for hiding this comment

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

This was a commit I made to fix some line endings. My editor is set to format json like this on purpose. It probably shouldn't happen here though you're right and we should uncommit this change.

Copy link

Choose a reason for hiding this comment

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

@BraidenCutforth My bad, your GitHub profile pictures look pretty much the same, I didn't see you had commits lol

/* User Agent Stylesheet Overrides */
body {
margin: 0;
font-family: 'Roboto arial';
Copy link

Choose a reason for hiding this comment

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

I would suggest placing any global changes to CSS like this in a separate PR; while it's a font you want to use, it's not specifically related to the component you are building. It's always best to write small PRs and separate things that are unrelated to each other.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was also me, and again should be separate PR. I think this might actually not be needed though.

In order to develop on this project you need the following:

- [node 12](https://nodejs.org/en/download/)
- [node 12](https://nodejs.org/en/download/)
Copy link

Choose a reason for hiding this comment

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

Looks like another format thing your editor did by accident 🙃

@daelynj
Copy link

daelynj commented Dec 10, 2019

It would be great to have the message body of this PR say what you've created and why you've created it, with a link to a ticket or something of the sort. That would save people a few minutes having to figure it out in the future! A descriptive PR title would help as well, such as "Implement Drawer component" rather than just "Drawer".

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants