Exercise7 by jobegrabber · Pull Request #9 · Systems-Development-and-Frameworks/dojo

jobegrabber · 2021-01-25T23:20:14Z

My review was for lichtow!

noeljns

I could find at least six stars: ⭐ ⭐⭐ ⭐⭐ ⭐

However there is still missing

PWA configuration

noeljns · 2021-01-26T19:31:18Z

webapp/components/LoginForm.vue

@@ -0,0 +1,111 @@
+<template>


Great ⭐ ⭐

For a login feature in your webapp including a Vue component...

noeljns · 2021-01-26T19:49:49Z

webapp/test/LoginForm.spec.js

@@ -0,0 +1,275 @@
+import '@testing-library/jest-dom'


Yeah, those two ⭐ ⭐ belong to the LoginForm component. Great that you wrote corresponding

...software tests.

noeljns · 2021-01-26T19:51:53Z

webapp/components/LoginMenu.vue

@@ -0,0 +1,34 @@
+<template>


Woho, two ⭐ ⭐

For a menu component which shows a login or logout button ...

noeljns · 2021-01-26T19:53:15Z

webapp/test/LoginMenu.spec.js

@@ -0,0 +1,137 @@
+import '@testing-library/jest-dom'
+import 'regenerator-runtime'


Thumbs up for those

software tests

of the menu component with its login and logout buttons. Now the two ⭐ ⭐ are fully earned!

noeljns · 2021-01-26T19:56:41Z

README.md


 ```
 $ (webapp) npm run build
 ```

+### Compiles and minifies for production (static build)
+
+```
+$ (webapp) npm run generate
+```
+
+### Serve in production
+
+```
+$ (webapp) npm run start
+```
+


This looks good! Another ⭐

For instructions in the README.md on how to build your webapp for production.

noeljns · 2021-01-26T20:02:01Z

webapp/components/NewsListItem.vue

+    <template v-if="isAuthenticated && userId === authorId">
+      <button disabled title="Not implemented yet">Edit</button>
+      <button @click="$emit('remove')">Remove</button>


This looks great! Another ⭐

For a delete and edit button that is only visible to the author of the post.

noeljns · 2021-01-26T20:04:37Z

webapp/components/NewsListItem.vue

+        :disabled="!isAuthenticated || userVote > 0"
+        :title="isAuthenticated ? null : 'Login to upvote'"
+        @click="$emit('upvote')"
+    >
+      Upvote
+    </button>
+    <button
+        :disabled="!isAuthenticated || userVote < 0"
+        :title="isAuthenticated ? null : 'Login to downvote'"
+        @click="$emit('downvote')"
+    >


This looks awesome as usual!
A final ⭐

For an upvote button that behaves according to the authentication state of your user

noeljns · 2021-01-26T20:07:26Z

webapp/nuxt.config.js

  // Build Configuration: https://go.nuxtjs.dev/config-build
-  build: {
-  }
+  build: {},


🔴 Maybe I am overlooking something, but I guess PWA is still missing.
Remember to catch your ⭐

For Lighthouse reporting that your production website is installable as PWA (except HTTPS).

jobegrabber · 2021-01-26T20:31:47Z

I could find at least six stars:

However there is still missing

PWA configuration

Thanks a lot for the review, very much appreciated!
My PWA configuration is here 😃 (reference)

roschaefer

Excellent! You received all 13/13 ⭐ in this submission. Congrats!

I really like that you wrote these storybooks. In my opinion, they really help to develop highly reusable and accessible components.

I would strongly suggest that give up the "should" in your test case description. It's just unnecessary.

⭐ For instructions in the README.md on how to build your webapp for production.

⭐ For no changes in "Files Changed" tab of the refactoring from vue-cli to create-nuxt-app. (See #1 in instructions)

⭐ ⭐ For the API connection between your front- and backend.

⭐ For your previous frontend tests still passing. Requests to the backend are mocked.

⭐ ⭐ For a login feature in your webapp including a Vue component and its software tests.

⭐ ⭐ For a menu component which shows a login or logout button and its software tests.

⭐ For an upvote button that behaves according to the authentication state of your user

⭐ For a delete and edit button that is only visible to the author of the post.

⭐ For Lighthouse reporting that your production website is installable as PWA (except HTTPS).

⭐ For requesting a review and reviewing another team's PR.

See my sugestions below.

roschaefer · 2021-02-13T14:12:13Z

.github/workflows/webapp.yml

+    branches: [ main, exercise7 ]
  pull_request:
-    branches: [ main, exercise3 ]
+    branches: [ main, exercise7 ]


What about testing all branches?

roschaefer · 2021-02-13T14:16:36Z

README.md

+```
+
 ### Run linter



There is a small typo further down this file 👇

$ docker run -p 7474:7474 -p 7687:7687 --env=NEO4J_AUTH=$NEO4J_USERNAME/$NEO4J_PASSWORD neo4j:4.2.1

Missing $

roschaefer · 2021-02-13T14:28:38Z

README.md

+### Compiles and minifies for production (static build)
+
+```
+$ (webapp) npm run generate


Running npm run generate fails with the following erros:

ERROR /index.stories 15:26:50 Error: render function or template not defined in component: anonymous at Qi (/home/robert/Development/Systems-Development-and-Frameworks/homework/webapp/node_modules/vue-server-renderer/build.prod.js:1:66721) at io (/home/robert/Development/Systems-Development-and-Frameworks/homework/webapp/node_modules/vue-server-renderer/build.prod.js:1:70594) at ro (/home/robert/Development/Systems-Development-and-Frameworks/homework/webapp/node_modules/vue-server-renderer/build.prod.js:1:70244) at eo (/home/robert/Development/Systems-Development-and-Frameworks/homework/webapp/node_modules/vue-server-renderer/build.prod.js:1:67491) at /home/robert/Development/Systems-Development-and-Frameworks/homework/webapp/node_modules/vue-server-renderer/build.prod.js:1:70711 at runNextTicks (internal/process/task_queues.js:58:5) at listOnTimeout (internal/timers.js:523:9) at processTimers (internal/timers.js:497:7) ERROR / 15:26:50 TypeError: Cannot read property 'length' of undefined at a.render (pages/index.vue?1e15:1:0) at a.t._render (/home/robert/Development/Systems-Development-and-Frameworks/homework/webapp/node_modules/vue/dist/vue.runtime.common.prod.js:6:35273) at /home/robert/Development/Systems-Development-and-Frameworks/homework/webapp/node_modules/vue-server-renderer/build.prod.js:1:70637

According to your .nuxt.config.js your deployment target is server. Please only include relevant deployment instructions in your README.md.

roschaefer · 2021-02-13T14:29:50Z

webapp/nuxt.config.js

+      background: '#ececec',
+      theme_color: '#0083e3',
+    },
+  },


❌ For Lighthouse reporting that your production website is installable as PWA (except HTTPS).

It's installable for me with npm run build and npm run start 😅

roschaefer · 2021-02-13T14:34:41Z

webapp/.storybook/main.js

+    '@storybook/addon-a11y',
+    '@storybook/addon-links',
+  ]
+})


Hm, you could have excluded this refactoring from the PR diff.

roschaefer · 2021-02-13T15:08:12Z

webapp/test/LoginForm.spec.js

+      await createLoginForm()
+      await wrapper.vm.$nextTick()
+
+      expect(wrapper.find('#form-login').element).not.toBeEmptyDOMElement()


I would suggest the assertions provided by vue-test-utils.

roschaefer · 2021-02-13T15:08:33Z

webapp/test/LoginForm.spec.js

+      expect(wrapper.html()).toMatchSnapshot()
+    })
+
+    it('should only enable the submit button if both email and password are provided', async () => {


Suggested change

it('should only enable the submit button if both email and password are provided', async () => {

it('only enables the submit button if both email and password are provided', async () => {

Be affirmative.

roschaefer · 2021-02-13T15:10:34Z

webapp/test/LoginForm.spec.js

+      await wrapper.vm.$nextTick()
+
+      expect(loginButton).toBeDisabled()
+
+      wrapper.find('#input-password').setValue('someSecret')
+      await wrapper.vm.$nextTick()
+
+      expect(loginButton).toBeEnabled()
+
+      wrapper.find('#input-password').setValue('')
+      await wrapper.vm.$nextTick()
+
+      expect(loginButton).toBeDisabled()


Try to have one expectation per test-case for readability and to avoid false-negatives (because of state altered in some special order). You can split this into multiple test cases and use beforeEach for a shared setup.

roschaefer · 2021-02-13T15:13:06Z

webapp/test/LoginForm.spec.js

+      expect(wrapper.html()).toMatchSnapshot()
+    })
+  })
+})


⭐ ⭐ For a login feature in your webapp including a Vue component and its software tests.

roschaefer · 2021-02-13T15:14:53Z

webapp/test/LoginMenu.spec.js

+    })
+  })
+})
+


⭐ ⭐ For a menu component which shows a login or logout button and its software tests.

jobegrabber · 2021-02-16T14:17:12Z

Thanks a lot for the thorough review, @roschaefer !

I would strongly suggest that give up the "should" in your test case description. It's just unnecessary.

I consider the tests as a set of specifications that the implementation(s) should fulfill (as in BDD) - in that sense, the word "should" (or "must") is part of the specification (or legal) lingo.
Ultimately, as many a thing, this is up for personal preference of course.

jobegrabber requested review from noeljns and powlaa January 26, 2021 00:24

jobegrabber force-pushed the exercise7 branch from 30ddd25 to 872052b Compare January 26, 2021 17:13

Base automatically changed from move-to-nuxt to main January 26, 2021 17:26

Jonas Grabber added 14 commits January 26, 2021 18:29

Add nuxt apollo + storybook, Login

a921023

Fix tests

0bf164b

Run workflow on exercise7 branch

855cc93

Fix lint command

4fffdae

Add webapp .gitignore

f4ab920

Fix tests

e99c5ea

Fix webapp workflow

3558279

Add posts merge cache policy

731e7da

Add test spec for LoginForm

ee4146b

Add LoginMenu spec

67d208c

Add custom PWA icon and favicon.ico

aa8894d

Switch to server target for webapp

4dc6b84

Update main README

30c1fe1

Remove unused directories

fc6da80

jobegrabber force-pushed the exercise7 branch from a14ada0 to fc6da80 Compare January 26, 2021 17:30

Jonas Grabber added 3 commits January 26, 2021 18:31

Add note to __typename being used

f4ab3f9

Use full paths for gql files to fix storybook

7486d49

Somewhat hackishly remove apollo from App stories

517d5d8

noeljns suggested changes Jan 26, 2021

View reviewed changes

noeljns mentioned this pull request Jan 26, 2021

Exercise 7 Systems-Development-and-Frameworks/lichtow#13

Open

Jonas Grabber added 2 commits January 26, 2021 23:22

Add note about context function for index posts

affc585

Add pwa manifest

94b1996

jobegrabber requested review from a team, medizinmensch and roschaefer February 1, 2021 00:10

roschaefer approved these changes Feb 13, 2021

View reviewed changes

		@@ -0,0 +1,137 @@
		import '@testing-library/jest-dom'
		import 'regenerator-runtime'

	it('should only enable the submit button if both email and password are provided', async () => {
	it('only enables the submit button if both email and password are provided', async () => {

Comments

Conversation

jobegrabber commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noeljns left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noeljns Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noeljns Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jobegrabber commented Jan 26, 2021

Uh oh!

roschaefer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jobegrabber commented Feb 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jobegrabber commented Jan 25, 2021 •

edited

Loading

noeljns left a comment •

edited

Loading

noeljns Jan 26, 2021 •

edited

Loading

noeljns Jan 26, 2021 •

edited

Loading