Feedback #5

cwesterduin · 2021-03-31T12:01:23Z

No description provided.

initial commit

Client

yourlocaldeveloper · 2021-03-31T13:15:21Z

README.md

+    -   Access: client on localhost:8080/ and server on localhost:3000/     
+-   Run `docker-compose down` to stop the services and keep data
+-   Run `docker-compose --volumes --remove-orphans` to stop the services and remove all artifacts


Not a massive thing but would of been nice to use the scripts similar to those found in the assessment for ease of access

yourlocaldeveloper · 2021-03-31T13:15:45Z

README.md

+## Bugs
+
+-   Should display an error if no post is found from the url


Good README with plenty of detail, nice work :)

yourlocaldeveloper · 2021-03-31T13:17:14Z

api/controllers/posts.js

+// trying to get all posts so that I can find out the id of each post
+async function index(req, res) {
+    try {
+        const posts = await Post.all;


Was wondering if it is necessary to make an all function given its not used in your website?

yourlocaldeveloper · 2021-03-31T13:17:34Z

api/controllers/posts.js

+    }
+}
+
+module.exports = { index, show, create };


Nice clean and concise controller file 💯

yourlocaldeveloper · 2021-03-31T13:18:12Z

api/dbConfig.js

+const { MongoClient } = require("mongodb");
+const connectionUrl = process.env.DB_CONNECTION;
+
+const dbName = process.env.DB_NAME;
+
+const init = async () => {
+    let client = await MongoClient.connect(connectionUrl);
+    console.log("connected to database!", dbName);
+    return client.db(dbName);
+};
+
+module.exports = { init };


Like the idea of using Mongo over PostgreSQL, DB config seems all in order! 👍

yourlocaldeveloper · 2021-03-31T13:18:44Z

api/models/Post.js

+    static get all() {
+        return new Promise(async (resolve, reject) => {
+            try {
+                const db = await init();
+                const postsData = await db.collection("posts").find().toArray();
+                const posts = postsData.map(
+                    (p) => new Post({ ...p, id: p._id })
+                );
+                resolve(posts);
+            } catch (err) {
+                console.log(err);
+                reject("Cannot retrieve all posts");
+            }
+        });
+    }


Good function but we don't think it gets used so is it necessary to keep in?

yourlocaldeveloper · 2021-03-31T13:20:42Z

api/models/Post.js

+    }
+}
+
+module.exports = Post;


Great model file, looks great and good use of spread operator for inserting data. Looks great, just one console.log that could be removed on line 49 👍

yourlocaldeveloper · 2021-03-31T13:21:38Z

client/index.html

+<!DOCTYPE html>
+<html lang="en">
+<head>
+    <meta charset="UTF-8">
+    <meta http-equiv="X-UA-Compatible" content="IE=edge">
+    <meta name="viewport" content="width=device-width, initial-scale=1.0">
+    <meta name="description" content="anonymous blogging application for ninjas!">
+    <title>Anoninja</title>
+    <link rel="stylesheet" href="static/css/index.css">
+    <link rel="icon" href="data:image/svg+xml,<svg xmlns=%22http://www.w3.org/2000/svg%22 viewBox=%220 0 100 100%22><text y=%22.9em%22 font-size=%2290%22>🐱‍👤</text></svg>">
+</head>
+<body>
+    <h1><a href="/">Anoninja</a></h1>
+    <div id="root"></div>
+</body>
+    <script defer src="bundle.js"></script>
+</html>


Nice simple HTML file, and like how you remembered to add the favicon 😄

yourlocaldeveloper · 2021-03-31T13:23:34Z

client/static/css/index.css

+@import url('https://fonts.googleapis.com/css2?family=Rock+Salt&display=swap');
+
+* {
+    box-sizing: border-box;
+}
+
+body {
+    font-family: 'Raleway', sans-serif;
+    background: rgb(34, 33, 31);
+    color: rgb(248, 244, 234);
+    margin: 0;
+    padding: 0;
+}
+
+h1, h1 > a {
+    font-family: 'Rock Salt', cursive;
+    color: rgb(235, 43, 43);
+    text-align: center;
+    text-decoration: none;
+}
+
+form {
+    display: flex;
+    flex-direction: column;
+    width: 100%;
+    justify-content: space-between;
+    align-items: center;
+    text-align: center;
+}
+
+input:not(input[type="submit"]), textarea {
+    background: transparent;
+    min-height: 45px;
+    outline: none;
+    border: none;
+    color: rgb(248, 244, 234);
+    font-size: 1.2rem;
+    width: 50%;
+    min-width: 300px;
+    font-family: 'Raleway', sans-serif;
+    margin-top: .25rem;
+    margin-bottom: 1rem;
+}
+input:first-of-type {
+    margin-top: 0 !important;
+    margin-bottom: 0 !important;
+}
+textarea {
+    min-height: 100px;
+    resize: vertical;
+}
+input[type="submit"] {
+    border: solid 1px rgb(221, 213, 213);
+    color: rgb(248, 244, 234);
+    cursor: pointer;
+    border-radius: 4px;
+    background: transparent;
+    padding: .5rem 2rem;
+    font-size: 1.2rem;
+}
+input[type="submit"]:hover {
+    background: #333;
+}
+
+#title {font-size: 2rem}
+#name {
+    font-family: 'Rock Salt', cursive;
+    color: rgb(235, 43, 43);
+}
+.post-cont{
+    width: 50%;
+    min-width: 300px;
+    margin: 0 auto;
+    font-size: 1.2rem;
+}
+.post#name {
+    margin: -2rem 0 0 2rem;
+    display: inline-block;
+}
+.post-cont{
+    width: 50%;
+    min-width: 300px;
+    margin: 0 auto;
+    font-size: 1.2rem;
+}
+.post-cont span {
+    font-size: 1rem;
+    color: rgb(221, 213, 213)
+}
+


Good styling given you weren't using a framework, good job looks great 💯

yourlocaldeveloper · 2021-03-31T13:25:07Z

client/static/js/api.js

+
+        const response = await fetch('http://localhost:3000/posts', options);
+        const data = await response.json();
+        window.location.hash = `#${data}`


Really picky here but you don't need the hashtag where you set the hash as it adds it in for you

yourlocaldeveloper · 2021-03-31T13:25:54Z

client/static/js/api.js

+
+module.exports = {
+    sendPost, getPost
+}


In general nice concise and well written async functions 😄

yourlocaldeveloper · 2021-03-31T13:28:29Z

client/static/js/layout.js

@@ -0,0 +1,61 @@
+const { sendPost, getPost } = require('./api.js')
+const { encode } = require('html-entities');


Really cool idea of thinking of decoding / encoding icons, emojis, nice work!

yourlocaldeveloper · 2021-03-31T13:29:17Z

client/static/js/layout.js

+    const root = document.getElementById('root')
+    root.innerHTML = ''
+    const id = window.location.hash.substring(1)
+    id.length > 0 ? renderPost(id) : renderForm()


Good use of a conditional operator, so much more cleaner than an if statement 😄

yourlocaldeveloper · 2021-03-31T13:29:35Z

client/static/js/layout.js

+    <div class="post-cont">
+    <h2>${encode(postData.title)}</h2>
+    <h3 class="post" id="name">${encode(postData.pseudonym)}</h3><span> ● ${prettyDate(postData.date)}</span>
+    <p>${encode(postData.content)}<p>
+    </div>`


Again great use of this encode function!

yourlocaldeveloper · 2021-03-31T13:30:05Z

client/static/js/layout.js

+function prettyDate(date) {
+    const jsDate = new Date(Date.parse(date))
+    const day = jsDate.getDate()	
+    const month = jsDate.getMonth() + 1
+    const year = jsDate.getFullYear()
+    return `${day}-${month}-${year}`
+}


Nice that you made your own function to do this, good separation of code so it can be re-used!

yourlocaldeveloper · 2021-03-31T13:33:23Z

client/static/js/layout.js

+    const fields = [
+        { tag: 'input', attributes: {autocomplete : "off", id: "title", required: "true", type: 'text', name: 'title', placeholder: 'Title' } },
+        { tag: 'input', attributes: {autocomplete : "off", id: "name", type: 'text', name: 'pseudonym', placeholder: 'Ninja name...' } },
+        { tag: 'textarea', attributes: { name: 'content', required: "true", placeholder: 'Ninja post...' } },
+        { tag: 'input', attributes: { type: 'submit', value: 'submit' } }
+    ]
+    const root = document.getElementById('root')
+    const form = document.createElement('form');
+        fields.forEach(f => {
+            const field = document.createElement(f.tag);
+            field.textContent = f.text;
+            Object.entries(f.attributes).forEach(([a, v]) => field.setAttribute(a, v))
+            form.appendChild(field);
+        })
+        form.onsubmit = sendPost;
+        root.appendChild(form);
+    }


Good creation of the form using a forEach, much nicer and makes life so much easier when you want to add more inputs. Great job 😄

yourlocaldeveloper · 2021-03-31T13:36:30Z

client/tests/api.spec.js

+
+
+
+})


No mention on how to run the tests? Look good so far though 👍

yourlocaldeveloper · 2021-03-31T13:37:27Z

db/seeds.js

+const db = connect("mongodb://localhost:27017/anoninjaDB");
+
+db.posts.drop();
+
+db.posts.insertMany([
+    {
+        title: "Ninjas",
+        pseudonym: "Ninja",
+        content: "Ninjas are better than Pirates",
+        date: new Date(),
+    },
+    {
+        title: "Pirates",
+        pseudonym: "Pirate",
+        content: "Pirates are better than ninjas",
+        date: new Date(),
+    },
+]);


Seeding looks good and works as it should, is cool to think your storing the Date() type, as opposed to a string, so you can manipulate it later 👍

yourlocaldeveloper · 2021-03-31T13:37:56Z

docker-compose.yaml

+version: "3"
+services:
+  client:
+    container_name: dev_client
+    build: ./client
+    image: node:12.18.4
+    ports: 
+      - 8080:8080
+    working_dir: /code
+    volumes:
+      - type: bind
+        source: ./client
+        target: /code
+    command: bash -c "npm install && npm run dev"
+
+  server:
+    image: node:12.18.4
+    working_dir: /code
+    ports: 
+      - 3000:3000
+    environment: 
+      - DB_NAME=anoninjaDB
+      - DB_CONNECTION=mongodb://anoninjaTeam:password@db:27017
+    depends_on:
+      - db
+    volumes:
+      - type: bind
+        source: ./api
+        target: /code
+    command: bash -c "npm install && npm run dev"
+
+  db:
+    image: mongo:latest
+    volumes:
+      - "dbdata:/var/lib/mongodb/data"
+      - "./db/seeds.js:/docker-entrypoint-initdb.d/seeds.js:ro"
+
+    environment: 
+      - MONGO_INITDB_ROOT_USERNAME=anoninjaTeam
+      - MONGO_INITDB_DATABASE=anoninjaDB
+      - MONGO_INITDB_ROOT_PASSWORD=password
+volumes:
+  dbdata:


Docker file worked first time, no issues!

yourlocaldeveloper

Great app guys, love the design and idea behind it. Perfect code just a few things (when being picky) that we pointed out. Checkout out all our comments and let us know if you have any questions or you want us to come and clarify anything 👍 Great work again 😄

JamesWheadon

Really nice design, well formatted code and all the functionality works which is always nice.

ros1995 and others added 30 commits March 30, 2021 15:01

Merge pull request #3 from roselynle/feedback

b69e605

initial commit

add base files

9c4f0b5

Merge branch 'main' of https://github.com/roselynle/anoninja

f6e535b

add base files

a26033d

initial client html and css

e62fc73

set up port to listen on

06595ba

add dependencies

2a42135

create server

8456b53

set up database connection file

53bc8d1

insert into database

df4b61e

update html for conditional rendering

7f7b929

functions for rendering form / post

1db2c43

add api functions

114122a

update to link separate file for the routes

2b481f7

create a seperate file for the routes

12d6211

add date to posts database

5b57c3f

create file for Post model

047f541

create file for controllers

4cdc7a7

add jest and local dev deps initial unit tests passing

a2dffd1

merge gitignore

c35335a

Merge pull request #4 from roselynle/client

80a5d3b

Client

Update .gitignore

2d1e584

add function for getting all posts

3cd8784

create docker compose file

1abfb01

start work on api tests

de03bfb

Merge branch 'main' of https://github.com/roselynle/anoninja into main

78cd69c

fix missing underscore

176c6a0

configure nodemon for windows and docker-compose runs api dev script

0af17d7

Merge branch 'main' of https://github.com/roselynle/anoninja into main

bd76559

update create Post method

41cb2b0

cwesterduin requested a review from JamesWheadon March 31, 2021 12:01

cwesterduin and others added 4 commits March 31, 2021 13:21

fix xss vulnerability in client

05b0f86

Merge branch 'main' of https://github.com/roselynle/anoninja into main

a762ad0

Update README.md

69ff53c

Update README.md

faf3f9d

yourlocaldeveloper self-requested a review March 31, 2021 12:55

yourlocaldeveloper reviewed Mar 31, 2021

View reviewed changes

api/controllers/posts.js

}

}

module.exports = { index, show, create };

Copy link

Collaborator

yourlocaldeveloper Mar 31, 2021

Choose a reason for hiding this comment

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

Nice clean and concise controller file 💯

yourlocaldeveloper reviewed Mar 31, 2021

View reviewed changes

client/tests/api.spec.js

})

Copy link

Collaborator

yourlocaldeveloper Mar 31, 2021

Choose a reason for hiding this comment

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

No mention on how to run the tests? Look good so far though 👍

yourlocaldeveloper reviewed Mar 31, 2021

View reviewed changes

yourlocaldeveloper approved these changes Mar 31, 2021

View reviewed changes

JamesWheadon approved these changes Mar 31, 2021

View reviewed changes

ros1995 added 3 commits April 1, 2021 09:57

install testing dependencies for api

552f0ba

Merge branch 'main' of https://github.com/roselynle/anoninja

05b3b69

add demo gif

d5b0b17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feedback #5

Feedback #5

cwesterduin commented Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper Mar 31, 2021

yourlocaldeveloper left a comment

JamesWheadon left a comment

		## Bugs

		- Should display an error if no post is found from the url

		@@ -0,0 +1,61 @@
		const { sendPost, getPost } = require('./api.js')
		const { encode } = require('html-entities');

Feedback #5

Are you sure you want to change the base?

Feedback #5

Conversation

cwesterduin commented Mar 31, 2021

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

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

yourlocaldeveloper left a comment

Choose a reason for hiding this comment

JamesWheadon left a comment

Choose a reason for hiding this comment