Alex-budko/issue491 #501
Alex-budko/issue491 #501
Conversation
There was a problem hiding this comment.
Left a few comments -- here's a summary
- We should lint front + backend
- Let's make sure this is visually consistent. Can you send a screenshot of how it looks in the penn courses channel on slack?
- There are some weird dependency adds + dev set up changes on the backend. Let's remove or chat about those.
backend/Pipfile
Outdated
| @@ -60,6 +60,9 @@ scikit-learn = "*" | |||
| pandas = "*" | |||
| python-dateutil = "*" | |||
| docutils = "*" | |||
| ics = "*" | |||
| psycopg2-binary = "*" | |||
There was a problem hiding this comment.
could we add this under dev packages at the very least?
| @@ -0,0 +1,27 @@ | |||
| # Generated by Django 4.1.5 on 2023-01-15 16:39 | |||
There was a problem hiding this comment.
can we merge this migration with the migrations on master?
backend/plan/urls.py
Outdated
| @@ -1,15 +1,15 @@ | |||
| from django.urls import include, path | |||
| from django.views.generic import TemplateView | |||
| from rest_framework import routers | |||
| from rest_framework_nested import routers | |||
There was a problem hiding this comment.
Wait why do we use rest_framework_nested here?
backend/plan/views.py
Outdated
| ) | ||
|
|
||
| def get(self, *args, **kwargs): | ||
| """ |
There was a problem hiding this comment.
we usually document this information in PcxAutoSchema -- see here for an example.
backend/plan/views.py
Outdated
| days.append(day_mapping[meeting.day]) | ||
| first_meeting = list(section.meetings.all())[0] | ||
|
|
||
| start_time = str(Meeting.int_to_time(first_meeting.start)) |
There was a problem hiding this comment.
Just a general question -- is there a reason we do this from the backend instead of the frontent?
There was a problem hiding this comment.
When I first built the feature I had all the logic done on the frontend, but Rohan suggested that logic be moved to the backend to simplify the frontend code and make it look more visually appealing
There was a problem hiding this comment.
Ok yeah I think the link is maybe more intuitive.
backend/plan/views.py
Outdated
| days = [] | ||
| for meeting in section.meetings.all(): | ||
| days.append(day_mapping[meeting.day]) | ||
| first_meeting = list(section.meetings.all())[0] |
There was a problem hiding this comment.
is any sorted order intended here?
frontend/package.json
Outdated
| @@ -20,5 +20,7 @@ | |||
| "hooks": { | |||
| "pre-commit": "lerna run --concurrency 1 precommit" | |||
| } | |||
| }, | |||
| "dependencies": { | |||
There was a problem hiding this comment.
do we need to have a dependencies obj?
| <hr /> | ||
|
|
||
| <Row className="columns has-text-centered"> | ||
| <div className="column"> |
There was a problem hiding this comment.
nit but I think we use 2 space indentation -- you also may need to lint the frontend
| onClick={async () => { | ||
| try { | ||
| await navigator.clipboard.writeText(url); | ||
| toast.info("Copied to clipboard!"); |
There was a problem hiding this comment.
how does this toast look? We should make sure it's visually consistent
|
One other thing I thought of -- it might be nice to have the calendar show up with a default name when you add the URL to google (not sure if it's possible or how much work it is -- just a thought) |
| 4. There's just one last complication. Due to some annoying details of Docker networking, you have to expose the server on IP address `0.0.0.0` inside the container, rather than `127.0.0.1` or `localhost` as is default (otherwise the server won't be accessible from outside of the container). To do this, instead of running `python manage.py runserver`, run `python manage.py runserver 0.0.0.0:8000`. In `Dockerfile.dev`, we automatically alias the command `runserver` to the latter, so in the container shell (in `/backend`, as is default) you can simply run the command `runserver` (no `python manage.py` necessary). | ||
| - To alias this command, run `echo "alias courses-compose='cd "$PWD"; docker-compose up'" >> ~/.zshrc; source ~/.zshrc` (replacing `~/.zshrc` with `~/.bashrc` or whatever configuration file your shell uses, if you don't use zsh). | ||
| - This will spin up a container from which you can run the server (with all required packages preinstalled). | ||
| 3. In a separate terminal (from any directory), run `docker exec -it backend-development-1 /bin/bash` to open a shell in the container (if this says "no such container", run `docker container ls` and use the name of whatever container most closely matches the `backend_development` image). Just like exiting a Pipenv shell, you can exit the container by pressing `Ctrl+D` (which sends the "end of transmission" / EOF character). |
There was a problem hiding this comment.
The numbering is off here, should be 2
| </a> | ||
| </BigText> | ||
| </div> | ||
| </Row> |
There was a problem hiding this comment.
Is there a reason why we're removing this? Seems like it would be helpful to include for people that don't know how to use ICS URL
Fixed the UI for PennCoursePlan to show a download icon next to each calendar, implying that we can independently download/import any one of our named schedules: