Updated home page, connected it with the API #18

tanya-kta · 2020-11-22T16:12:16Z

Hi! This is an update of the home page to how it was previously designed and its connection with the API

kolayne

Hi. Unfortunately, there are some new issues in your PR, some of them are more serious than the others. Maybe it's too late (or probably not), but apparently it might be a good idea to split the PR into two smaller ones, for example, one introducing static HTML pages and the other one adding JS. But if you decide to leave it as one PR, I think there should be 4 approvals before we merge it, because there are updates (and therefore there are issues) of very different kinds (thank god there are no changes to the API or database schemes in this PR)

kolayne · 2020-12-06T17:53:16Z

src/webhandlers.go

@@ -172,6 +172,17 @@ func main() {
 	r.HandleFunc("/api/days/brief", HandleApiDaysBrief).Methods("GET")
 	//r.HandleFunc("/api/days/{id:[0-9]+}")

+	//r.Handle("/src", )


I think this is a mistake

kolayne · 2020-12-06T17:54:20Z

src/webhandlers.go

+	r.HandleFunc("/scripts/*", func(w http.ResponseWriter, r *http.Request) {
+		r.URL.Path = r.URL.Path[len("/scripts"):]
+		http.FileServer(http.Dir("src/scripts")).ServeHTTP(w, r)
+	})
+


This shouldn't be here either... The following handler does everything you want

kolayne · 2020-12-06T17:55:29Z

src/templates/index.html

+<title>Welcome to Life Calender!</title>
+<meta charset="UTF-8">
+<meta name="viewport" content="width=device-width, initial-scale=1">
+<link rel="stylesheet" href="https://www.w3schools.com/w3css/4/w3.css">
+<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Roboto">
+<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Montserrat">
+<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.7.0/css/font-awesome.min.css">
+<style>
+    .w3-sidebar a {font-family: "Roboto", sans-serif}
+    body,h1,h2,h3,h4,h5,h6,.w3-wide {font-family: "Montserrat", sans-serif;}
+    .square {
+        height: 70px;
+        width: 70px;
+        background-color: #0b6;
+    }
+</style>


Where is <head>?!

kolayne · 2020-12-06T17:56:53Z

src/templates/index.html

+{{if not .}}
+    Unfortunately, you are not logged in now :(<br>
+    <a href="/login">Log in!</a>
+{{else}}


It will look better if you will use if condition ... else ... instead of if not condition ... else ...

kolayne · 2020-12-06T17:58:27Z

src/templates/index.html

+    <a href="/login">Log in!</a>
+{{else}}
+    <!-- Sidebar/menu -->
+    <nav class="w3-sidebar w3-bar-block w3-white w3-collapse w3-top" style="z-index:3;width:265px" id="mySidebar">


(not sure) Probably id="mySidebar" shouldn't get into master (there are similar issues among this file)

kolayne · 2020-12-06T18:20:02Z

src/templates/index.html

+    <a href="/login">Log in!</a>
+{{else}}
+    <!-- Sidebar/menu -->
+    <nav class="w3-sidebar w3-bar-block w3-white w3-collapse w3-top" style="z-index:3;width:265px" id="mySidebar">


What is z-index for here? I don't see any other usages of z-index on this page...

kolayne · 2020-12-06T18:27:10Z

src/scripts/index.js

+
+    methods: {
+        colorToHex(color) {
+            function compToHex(c) {


What does the function name mean?

kolayne · 2020-12-06T18:32:49Z

src/scripts/index.js

+
+        isDayFilled(day) {
+            for(let filledDay of this.days) {
+                if(Date.parse(filledDay.date) === Date.parse(day.toISOString())) {


I don't think Date.parse should be called each time. You should (probably) store dates as javascript objects, not as strings. And there definitely should be a better way of converting day from a Date object to a timestamp, so at least the right value should be calculated without a call to Date.parse and outside of the loop

Still not resolved

kolayne · 2020-12-06T18:49:12Z

src/scripts/index.js

+            let threeWeeks = [];
+            for(let i = 2; i >= 0; --i) {
+                let currentWeek = [];
+                for(let j = 6; j >= 0; --j) {
+                    currentWeek.push(new Date(closestSunday));
+                    currentWeek[6 - j].setUTCDate(closestSunday.getUTCDate() - j - i * 7);
+                }
+                threeWeeks.push(currentWeek);
+            }


I don't like these loops are going backward and you are pushing to these arrays because you know the arrays' sizes in advance

kolayne · 2020-12-06T19:20:58Z

src/scripts/index.js

+    mounted() {
+        axios.get("/api/days/brief").then((response) => {
+            this.days = response.data;
+        });


I guess, should be something like this.days = response.data.days;. Also, don't forget to handle an API error if it occurs (i. e. response.data.ok is false). At least give some message about it (I think an alert should be enough for now)

tanya-kta · 2020-12-14T08:08:35Z

Hello, @kolayne! It seems that some issues in backend need to be fixed after merging master. They are those two you have mentioned first in your review, I suppose. Please, solve this conflict

kolayne · 2020-12-14T08:18:45Z

@tanya-kta, I don't understand what kind of changes I've mentioned in my review you are talking about. I think I've resolved the merge conflicts, please, check it out

… in `getDisplayedWeeks`

DarkSquirrelComes

Use array.map instead of for-loop (check for a comment)

DarkSquirrelComes · 2021-01-25T08:10:41Z

src/scripts/index.js

+    mounted() {
+        axios.get("/api/days/brief").then((response) => {
+            if (response.data.ok) {
+                this.days = response.data.days;


https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map

DarkSquirrelComes

ok

kolayne

Sorry, there still are some issues... But you seem to work well!

kolayne · 2021-01-26T10:36:52Z

src/scripts/index.js

+
+        isDayFilled(day) {
+            for(let filledDay of this.days) {
+                if(Date.parse(filledDay.date) === Date.parse(day.toISOString())) {


Still not resolved

kolayne · 2021-01-26T10:37:54Z

src/scripts/index.js

+
+        getFilledDayColor(day) {
+            for(let filledDay of this.days) {
+                if(Date.parse(filledDay.date) === Date.parse(day.toISOString())) {


Same as above

kolayne · 2021-01-26T10:38:40Z

src/scripts/index.js

+            for(let filledDay of this.days) {
+                if(Date.parse(filledDay.date) === Date.parse(day.toISOString())) {


Because you have to similar pieces of code, I would move them to a separate function - something like findDayByDate

kolayne · 2021-01-26T10:43:46Z

src/scripts/index.js

+            let closestSunday = new Date();
+            closestSunday = new Date(Date.UTC(closestSunday.getFullYear(), closestSunday.getMonth(),
+                closestSunday.getDate() + 7 - closestSunday.getDay()));


I don't think if it is worth renaming the variable, but I would at least add a comment that this is not the closest Sunday, but the closest Sunday in future. It is not obvious at all unless you delve into the expression used to calculate it

kolayne · 2021-01-26T10:46:07Z

src/scripts/index.js

+            if (response.data.ok) {
+                this.days = response.data.days;
+                this.days.map((day) => {
+                    day.data = new Date(day.data);


The field datA of day is a datE? It is either a dangerous typo or a very bad field name

Field data of day is a field mentioned in API docs as a field of API responce, when Date is a javascript object. If it bothers you, this can be fixed and field data can be renamed to date, but I think not in this PR. You may open an issue

@tanya-kta, I don't understand what you're talking about. I can't find the word "data" in the /api/days/brief's documentation

Note: such key is mentioned in the docs of /api/activities (and it also exists in /api/emotions, because these methods have the same interface), but this piece of code doesn't seem to be anyhow related to those methods

Yeah, sorry, I wasn't right. That's a bug

kolayne · 2021-01-26T10:48:26Z

src/templates/index.html

+		.w3-sidebar a {font-family: "Roboto", sans-serif}
+		body,h1,h2,h3,h4,h5,h6,.w3-wide {font-family: "Montserrat", sans-serif;}
+		.square {
+            width: 10%;
+            background-color: #0b6;
+        }


Something is wrong with indentation/spaces

kolayne · 2021-01-26T10:49:13Z

src/templates/index.html

+	</style>
+</head>
+<body class="w3-content" style="max-width:98%">
+{{if .}}


A comment must be here!

kolayne · 2021-01-26T13:51:22Z

src/templates/index.html

+    <script src="https://unpkg.com/vue"></script>
+    <script src="https://unpkg.com/axios/dist/axios.min.js"></script>
+    <script src="/scripts/index.js"></script>


@tanya-kta,

The fact you don't know which rel to use should not affect the decision whether this should be moved to <head> or not, of course. I'm asking because I am not closely familiar with the rules of placing scripts on web pages, but I have seen such scripts loaded with the link tag inside of head, that's why I am proposing this change

Answering your question, rel="preload" is usually used for js

kolayne · 2021-01-26T13:53:44Z

src/templates/index.html

+        function w3_open() {
+            document.getElementById("UserSidebar").style.display = "block";
+            document.getElementById("Overlay").style.display = "block";
+        }
+
+        function w3_close() {
+            document.getElementById("UserSidebar").style.display = "none";
+            document.getElementById("Overlay").style.display = "none";
+        }


I don't like these functions' names. As far as I understand w3 relates to the w3css framework we are using here. But these functions themselves have nothing to do with w3! I guess this was just copy-pasted from an example/tutorial. If so, the functions should be renamed so that their names reflect the actual purpose of these functions (open/close sidebar menu?)

kolayne · 2021-01-26T13:54:28Z

src/webhandlers.go

+	ui.PathPrefix("/scripts/").Handler(http.StripPrefix("/scripts/", http.FileServer(http.Dir("src/scripts")))).
+		Methods("GET")


@DarkSquirrelComes, how do you like the formatting? Is it ok?

tanya-kta and others added 4 commits November 22, 2020 17:00

Updated home page; first VueJS using

37f78f4

Connected to API; changed dates comparison

d456f7d

Added static src/scripts folder routing + TODO

4c12d66

Fixed issues found during Codacy checks

87e9cf2

tanya-kta force-pushed the first_frontend branch from f2ad81e to 87e9cf2 Compare December 6, 2020 16:05

tanya-kta marked this pull request as ready for review December 6, 2020 16:20

tanya-kta requested review from kolayne, DarkSquirrelComes, zhora15 and FixedOctocat December 6, 2020 16:21

kolayne requested changes Dec 6, 2020

View reviewed changes

kolayne added API / back-end Relates to the API or the back-end Front-end Relates to the front-end labels Dec 7, 2020

Merge remote-tracking branch 'origin/master' into first_frontend

1fd19d4

Fixed merge conflicts and removed debugging handlers

2b75617

tanya-kta and others added 5 commits December 14, 2020 14:44

Changed the condition of whether the user is logged in or not

9ab302a

Renamed functions RGBColorToHex and convertToHex; rewritten loops…

4d42c7b

… in `getDisplayedWeeks`

Added checking field ok in API response

54c9f4e

Fixed issues found during Codacy checks

56c606a

Updated home page style

4d115bc

FixedOctocat force-pushed the first_frontend branch from 41213ac to 4d115bc Compare January 25, 2021 08:01

tanya-kta requested a review from kolayne January 25, 2021 08:03

DarkSquirrelComes requested changes Jan 25, 2021

View reviewed changes

DarkSquirrelComes approved these changes Jan 25, 2021

View reviewed changes

Fixed issues found during the review

53323ab

tanya-kta force-pushed the first_frontend branch from 4b1ed99 to 53323ab Compare January 25, 2021 09:01

kolayne requested changes Jan 26, 2021

View reviewed changes

Rewritten getFilledDayColor; minor docs improvements

eb394e2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated home page, connected it with the API #18

Updated home page, connected it with the API #18

tanya-kta commented Nov 22, 2020

kolayne left a comment •

edited

Loading

kolayne Dec 6, 2020

kolayne Dec 6, 2020

kolayne Dec 6, 2020

kolayne Dec 6, 2020

kolayne Dec 6, 2020

kolayne Dec 6, 2020

kolayne Dec 6, 2020

kolayne Dec 6, 2020

kolayne Jan 26, 2021

kolayne Dec 6, 2020

kolayne Dec 6, 2020

tanya-kta commented Dec 14, 2020

kolayne commented Dec 14, 2020

DarkSquirrelComes left a comment

DarkSquirrelComes Jan 25, 2021

DarkSquirrelComes left a comment

kolayne left a comment

kolayne Jan 26, 2021

kolayne Jan 26, 2021

kolayne Jan 26, 2021

kolayne Jan 26, 2021

kolayne Jan 26, 2021

tanya-kta Feb 8, 2021

kolayne Feb 8, 2021

tanya-kta Feb 15, 2021

kolayne Jan 26, 2021

kolayne Jan 26, 2021

kolayne Jan 26, 2021

kolayne Jan 26, 2021 •

edited

Loading

kolayne Jan 26, 2021

		for(let filledDay of this.days) {
		if(Date.parse(filledDay.date) === Date.parse(day.toISOString())) {

		ui.PathPrefix("/scripts/").Handler(http.StripPrefix("/scripts/", http.FileServer(http.Dir("src/scripts")))).
		Methods("GET")

Updated home page, connected it with the API #18

Are you sure you want to change the base?

Updated home page, connected it with the API #18

Conversation

tanya-kta commented Nov 22, 2020

kolayne left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanya-kta commented Dec 14, 2020

kolayne commented Dec 14, 2020

DarkSquirrelComes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarkSquirrelComes left a comment

Choose a reason for hiding this comment

kolayne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolayne Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolayne left a comment •

edited

Loading

kolayne Jan 26, 2021 •

edited

Loading