Closed kdumais111 closed 1 week ago
kdumais111
commented
2 weeks ago 
meganhmoore
commented
2 weeks ago Looks like things need to be linted, run ‘black app/‘ to lint every file in the app directory
kdumais111
commented
2 weeks ago @meganhmoore lost a { in the merge which stopped black from parsing. Should be good now
meganhmoore
commented
2 weeks ago thanks for linting so quickly! It seems like things are now breaking on a bunch of things with the survey models, but I imagine those aren't related to your work? Not really sure how those changes got in and I'm not totally sure how to get rid of all of those changes easily on this branch, if you do, be my guest!
Otherwise maybe create a new branch just from main with just the heatmap.js and other js/html related things for that work so that production won't be broken by things you aren't in charge of.
kdumais111
commented
2 weeks ago @meganhmoore It was such a complex merge, felt like it was easier for me to fix the problem as long as its okay with @JPMartinezClaeys and @lisette-solis, as it was just that the survey models were missing a required max_char parameter in main, which i set based on the field type. All the tests run now.
JPMartinezClaeys
commented
1 week ago @meganhmoore It was such a complex merge, felt like it was easier for me to fix the problem as long as its okay with @JPMartinezClaeys and @lisette-solis, as it was just that the survey models were missing a required max_char parameter in main, which i set based on the field type. All the tests run now.
@lisette-solis and I made significant changes to the models.py file yesterday to move from a single answer per user to a single answer per user-route. I know it will be a bit messy, but if merging this, when getting a merge conflict please just accept all incoming (?) changes so they can match the tables definition.
lisette-solis
commented
1 week ago If possible, can you make the size and alignment on the maps the same. I think the width of the first one but maybe a bit more square would be idea:
kdumais111
commented
1 week ago [cid:CE308717-C257-4AE8-909C-5CBB37D06790]
done!
On May 19, 2024, at 5:17 PM, Lisette Solis @.**@.>> wrote:
Can you make the size and alignment on the maps the same: image.png (view on web)https://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/assets/111702116/8fb2b8da-c34f-4252-b413-f9d961501813__;!!BpyFHLRN4TMTrA!4q96yZmlQWwkN8__rbWIDKwnJ54Z35J_80PkYQ0PWfnHoJW4HFf_W88AzgQGN3PiHJ6Q4wjn8AKI0kS-oCZe4qDcpQE$
— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*issuecomment-2119377049__;Iw!!BpyFHLRN4TMTrA!4q96yZmlQWwkN8__rbWIDKwnJ54Z35J_80PkYQ0PWfnHoJW4HFf_W88AzgQGN3PiHJ6Q4wjn8AKI0kS-oCZeaaAhgUg$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/A3MRITXNABM3O3ZCNKBY26LZDEQIPAVCNFSM6AAAAABH57H7FGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZGM3TOMBUHE__;!!BpyFHLRN4TMTrA!4q96yZmlQWwkN8__rbWIDKwnJ54Z35J_80PkYQ0PWfnHoJW4HFf_W88AzgQGN3PiHJ6Q4wjn8AKI0kS-oCZeBePLrFE$. You are receiving this because you authored the thread.Message ID: @.***>
lisette-solis
commented
1 week ago Map resizing looks great! thank you :)
kdumais111
commented
1 week ago Hopefully this was a one time thing— We fail all the linting tests if we don’t run on things that weren’t linted and placed in main, so it shouldn’t be an ongoing problem if everyone is linting. But if we are expecting everyone to pass all tests in order to push to main, worth discussing what protocol should be here.
K
On May 19, 2024, at 6:19 PM, Lisette Solis @.**@.>> wrote:
@lisette-solis requested changes on this pull request.
Looks good overall! We should discuss linting tomorrow because linting all of /app included a bunch of files that probably shouldn't be
In app/route_rangers_api/models.pyhttps://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*discussion_r1606114533__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rh70b7U8o$:
# Page 2:
- trip_frequency = models.IntegerField(choices=TRIP_FREQ, null=True)
- trip_tod = models.IntegerField(choices=TIME_OF_DAY, null=True)
- trip_time = models.IntegerField(null=True)
- modes_of_transit = models.IntegerField(choices=MODES_OF_TRANIST, null=True)
- TRIP_FREQ = {
eeck this is using the earlier version of this code so please just keep all the current versions of this file when merging as JP mentioned
In app/route_rangers_api/static/cards.jshttps://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*discussion_r1606114666__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rh7BX9GRs$:
@@ -0,0 +1,20 @@ +function updateCards(buttonId, element) {
- var buttonText = element.textContent;
- document.getElementById(buttonId).textContent = buttonText;
- if (buttonId === "daysDropdown") {
- days = buttonText;
- } else if (buttonId === "seasonDropdown") {
- seasons = buttonText;
- } else {
- type = buttonText;
- }
- console.log(days, seasons, type);
Delete the console.log from production code please
In app/route_rangers_api/static/heatmap.jshttps://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*discussion_r1606114871__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhDyOEsCk$:
-function heatmaps(filepath, coordinates, label) {
- var geojson
- fetch(filepath)
- .then(response => response.json())
- .then(data => {
- var geojson = L.geoJson(data, {
- style: style,
- onEachFeature: onEachFeature
- }).addTo(heatmap); +function heatmaps(
- filepath,
- coordinates,
- label,
- scale,
- variable,
- colorscale = [
this could probably be constant inside the function if the colors aren't changing
In app/route_rangers_api/static/map.jshttps://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*discussion_r1606115525__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhbCLNUu4$:
\ No newline at end of file +}
these also look like linting changes so you should probably keep whatever is in main to not change Matt's code. This is super, super, super low priority @meganhmoorehttps://urldefense.com/v3/__https://github.com/meganhmoore__;!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhg1dL_KE$ but some of the linting doesn't make sense for Javascript (i.e. changing ' to " because ' are standard in JS). We can look at the data viz linting if we're feeling nerdy.
In app/route_rangers_api/views.pyhttps://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*discussion_r1606117492__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rha7lPe7Y$:
@@ -38,12 +40,11 @@ def home(request):
def about(request): context = {"cities_class": "cs-li-link", "about_class": "cs-li-link cs-active"}
Potentially merge craziness? But I think this needs to be changed back to about.html because I see this right now image.png (view on web)https://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/assets/111702116/21445f76-49e0-4523-bd42-e73dd86c260d__;!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhkJwGTDw$
On app/route_rangers_api/templates/test.htmlhttps://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*discussion_r1606121343__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhEHdMdNY$:
I don't think this should be pushed to main if we can avoid it
— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*pullrequestreview-2065154606__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhkP5upJY$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/A3MRITRUNUUR4CPG5ND2XI3ZDEXPZAVCNFSM6AAAAABH57H7FGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANRVGE2TINRQGY__;!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhgfqbt-Y$. You are receiving this because you authored the thread.Message ID: @.***>
lisette-solis
commented
1 week ago Makes sense! Definitely more of a Megan question and team discussion.
From: Katherine Dumais @.> Date: Sunday, May 19, 2024 at 6:24 PM To: uchicago-capp-30320/RouteRangers @.> Cc: Lisette Solis @.>, Mention @.> Subject: Re: [uchicago-capp-30320/RouteRangers] 113 create dropdown for html cards/ lint/ edit heatmap function (PR #137) Hopefully this was a one time thing— We fail all the linting tests if we don’t run on things that weren’t linted and placed in main, so it shouldn’t be an ongoing problem if everyone is linting. But if we are expecting everyone to pass all tests in order to push to main, worth discussing what protocol should be here.
K
On May 19, 2024, at 6:19 PM, Lisette Solis @.**@.>> wrote:
@lisette-solis requested changes on this pull request.
Looks good overall! We should discuss linting tomorrow because linting all of /app included a bunch of files that probably shouldn't be
In app/route_rangers_api/models.py<https://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*discussion_r1606114533__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rh70b7U8o$%3E:
Page 2:
- trip_frequency = models.IntegerField(choices=TRIP_FREQ, null=True)
- trip_tod = models.IntegerField(choices=TIME_OF_DAY, null=True)
- trip_time = models.IntegerField(null=True)
- modes_of_transit = models.IntegerField(choices=MODES_OF_TRANIST, null=True)
- TRIP_FREQ = {
eeck this is using the earlier version of this code so please just keep all the current versions of this file when merging as JP mentioned
In app/route_rangers_api/static/cards.js<https://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*discussion_r1606114666__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rh7BX9GRs$%3E:
@@ -0,0 +1,20 @@ +function updateCards(buttonId, element) {
- var buttonText = element.textContent;
- document.getElementById(buttonId).textContent = buttonText;
- if (buttonId === "daysDropdown") {
- days = buttonText;
- } else if (buttonId === "seasonDropdown") {
- seasons = buttonText;
- } else {
- type = buttonText;
- }
- console.log(days, seasons, type);
Delete the console.log from production code please
In app/route_rangers_api/static/heatmap.js<https://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*discussion_r1606114871__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhDyOEsCk$%3E:
-function heatmaps(filepath, coordinates, label) {
- var geojson
- fetch(filepath)
- .then(response => response.json())
- .then(data => {
- var geojson = L.geoJson(data, {
- style: style,
- onEachFeature: onEachFeature
- }).addTo(heatmap); +function heatmaps(
- filepath,
- coordinates,
- label,
- scale,
- variable,
- colorscale = [
this could probably be constant inside the function if the colors aren't changing
In app/route_rangers_api/static/map.js<https://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*discussion_r1606115525__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhbCLNUu4$%3E:
\ No newline at end of file +}
these also look like linting changes so you should probably keep whatever is in main to not change Matt's code. This is super, super, super low priority @meganhmoore<https://urldefense.com/v3/__https://github.com/meganhmoore__;!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhg1dL_KE$%3E but some of the linting doesn't make sense for Javascript (i.e. changing ' to " because ' are standard in JS). We can look at the data viz linting if we're feeling nerdy.
In app/route_rangers_api/views.py<https://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*discussion_r1606117492__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rha7lPe7Y$%3E:
@@ -38,12 +40,11 @@ def home(request):
def about(request): context = {"cities_class": "cs-li-link", "about_class": "cs-li-link cs-active"}
Potentially merge craziness? But I think this needs to be changed back to about.html because I see this right now image.png (view on web)<https://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/assets/111702116/21445f76-49e0-4523-bd42-e73dd86c260d__;!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhkJwGTDw$%3E
On app/route_rangers_api/templates/test.html<https://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*discussion_r1606121343__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhEHdMdNY$%3E:
I don't think this should be pushed to main if we can avoid it
— Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/uchicago-capp-30320/RouteRangers/pull/137*pullrequestreview-2065154606__;Iw!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhkP5upJY$%3E, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/A3MRITRUNUUR4CPG5ND2XI3ZDEXPZAVCNFSM6AAAAABH57H7FGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANRVGE2TINRQGY__;!!BpyFHLRN4TMTrA!_7QP7hMF1IMtDucXklhKKP6_WEh-exYmrNANTiBASikRafrLnuNV-9EJVpOvJGoJFdm-v7g-liBvXL9cw_rhgfqbt-Y$%3E. You are receiving this because you authored the thread.Message ID: @.***>
— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/uchicago-capp-30320/RouteRangers/pull/137*issuecomment-2119396511__;Iw!!BpyFHLRN4TMTrA!_ON55YcatUFLBlii7tu6oGu36dkuuT-vKsYjFsxtepVRhx6JLgNrl4iBFSjXa3ExaZmTSSgUQ9DKCxMLMY0eoaP2f029cR0$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/A2UHAZCWNLOLZ4SMX762R4LZDEYBLAVCNFSM6AAAAABH57H7FGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZGM4TMNJRGE__;!!BpyFHLRN4TMTrA!_ON55YcatUFLBlii7tu6oGu36dkuuT-vKsYjFsxtepVRhx6JLgNrl4iBFSjXa3ExaZmTSSgUQ9DKCxMLMY0eoaP2whaxCpM$. You are receiving this because you were mentioned.Message ID: @.***>