Exercise8 #10

jobegrabber · 2021-02-01T00:09:50Z

No description provided.

Use TailwindCSS 2 for webapp (requires move to yarn)

jobegrabber

roschaefer

Hey @Systems-Development-and-Frameworks/dojo you received all 5/5 ⭐ in this exercise. The result doesn't look as visually appealing to what I saw already. But I'm not good in design and visuals myself 😞

The sidebar menu is always open in my browser (Firefox, Chromium). Is this a bug or not yet implemented?

⭐ For a header and it's responsiveness (direct links vs. hamburger menu)

⭐ For a content section and it's content items

⭐ For a responsive content section (3-column vs 1-column layout)

⭐ For responsive content items (1-column vs. 2-column layout)

⭐ For a footer

Here's a security advice: You shouldn't distinguish between "E-Mail not found" and "Password incorrect" to prevent an attacker from doing a user listing.

In total you received all 70/70 ⭐ during the entire course - outstanding!

roschaefer · 2021-02-16T12:44:26Z

webapp/components/BasicButton.vue

+      :href="href"
+      class="inline-flex m-1.5 px-2.5 py-2 rounded-lg shadow-md text-white text-center
+      cursor-pointer justify-center items-center"
+      :class="(!disabled ? `bg-${color}-500 hover:bg-${color}-600` : `bg-gray-500`) + (bold ? ' font-semibold' : '')">


PurgeCSS will not be able to pick up the class names here. Don't use string interpolation like that in tailwind, instead, enumerate the classes.

roschaefer · 2021-02-16T12:45:27Z

webapp/components/NavBar.vue

+    </nav>
+    <div
+        class="absolute z-20 w-full h-full bg-gray-50 opacity-50"
+        :class="sidebarOpen ? '' : 'hidden'"


Suggested change

:class="sidebarOpen ? '' : 'hidden'"

:class="{sidebarOpen : 'hidden'}"

roschaefer · 2021-02-16T12:45:39Z

webapp/components/NavBar.vue

+        DojoNews
+      </section>
+      <section class="hidden lg:block lg:flex lg:justify-between lg:bg-transparent">
+        <NavMenu :is-authenticated="isAuthenticated" @logout="logout()"></NavMenu>


Suggested change

<NavMenu :is-authenticated="isAuthenticated" @logout="logout()"></NavMenu>

<NavMenu :is-authenticated="isAuthenticated" @logout="logout"></NavMenu>

roschaefer · 2021-02-16T12:46:36Z

webapp/components/NavBar.vue

+        :class="sidebarOpen ? 'left-0' : '-left-1/2 md:-left-1/3'"
+    >
+      <button class="self-end" @click="sidebarOpen = false">[x]</button>
+      <NavMenu :vertical="true" :is-authenticated="isAuthenticated" @logout="logout(); sidebarOpen = false"


Please use a dedicated method when you have to split commands with ;.

roschaefer · 2021-02-16T12:47:12Z

webapp/components/NavBar.vue

+    async logout() {
+      this.unsetToken()
+      await this.$apolloHelpers.onLogout()
+      await this.$nuxt.refresh()


Components should be reactive. Refetching data manually shouldn't be necessary.

It shouldn't, but somehow the tests fail otherwise 🤷‍♂️

roschaefer · 2021-02-16T12:47:48Z

webapp/components/NavMenu.vue

+<style scoped>
+
+</style>


Suggested change

<style scoped>

</style>

roschaefer · 2021-02-16T12:49:54Z

webapp/components/NuxtLinkButton.vue

@@ -0,0 +1,30 @@
+<template>
+  <nuxt-link @click.native="$emit('click')" :to="this.to" tag="div" class="flex">
+    <BasicButton :color="this.color" class="flex-grow" :href="this.href">


Is this necessary? 😕

roschaefer · 2021-02-16T12:52:22Z

webapp/pages/index.vue

  },
  computed: {
-    sortedNewsListItems () {
+    sortedNewsListItems() {


Did you change your lint configurations? 🤔

Having these changes creates noise and distracts from relevant changes. You probably want to exclude those changes from the PR being reviewed.

roschaefer · 2021-02-16T12:52:33Z

webapp/pages/login.vue

+  <main>
    <LoginForm/>
-  </div>
+  </main>


jobegrabber · 2021-02-16T13:57:48Z

Thanks for the review @roschaefer !

The sidebar menu is always open in my browser (Firefox, Chromium). Is this a bug or not yet implemented?

How do you run the app? To use Tailwind 2 I had to switch to yarn on the webapp side.
With yarn run dev the sidebar works as expected for me.

Here's a security advice: You shouldn't distinguish between "E-Mail not found" and "Password incorrect" to prevent an attacker from doing a user listing.

For this toy application, yes, that would prevent an attacker from listing user email addresses.
But for pretty much any proper application (which allows open registration of new users) this won't help really; the attacker can probe for user email addresses during registration.

I think rate-limits and captchas for both login and registration are better for providing security.

roschaefer · 2021-02-16T14:21:38Z

Thanks for the review @roschaefer !

The sidebar menu is always open in my browser (Firefox, Chromium). Is this a bug or not yet implemented?

How do you run the app? To use Tailwind 2 I had to switch to yarn on the webapp side.
With yarn run dev the sidebar works as expected for me.

Here's a security advice: You shouldn't distinguish between "E-Mail not found" and "Password incorrect" to prevent an attacker from doing a user listing.

For this toy application, yes, that would prevent an attacker from listing user email addresses.
But for pretty much any proper application (which allows open registration of new users) this won't help really; the attacker can probe for user email addresses during registration.

I think rate-limits and captchas for both login and registration are better for providing security.

I'm pretty sure I used npm because you have it in your README.md and you have package-lock.json and no yarn.lock in your webapp/.

Both for password reset and signup you don't have to expose user information. E.g. if the email address exists or not, you can send "We have sent an email with further instructions to ". There are best practices for that on the internet.

jobegrabber · 2021-02-16T15:53:17Z

I'm pretty sure I used npm because you have it in your README.md and you have package-lock.json and no yarn.lock in your webapp/.

yarn.lock is there, but I did forget to update the README, thanks!! 😄

Both for password reset and signup you don't have to expose user information. E.g. if the email address exists or not, you can send "We have sent an email with further instructions to ". There are best practices for that on the internet.

Ah yes, that's a nice way to get around that issue, thanks!

Add responsive About Us stub, move webapp to yarn

f4f7d0b

Use TailwindCSS 2 for webapp (requires move to yarn)

jobegrabber requested review from medizinmensch, noeljns, powlaa and roschaefer February 1, 2021 00:09

Update webapp workflow

35e9e1a

jobegrabber changed the base branch from main to exercise7 February 1, 2021 00:42

Bit of DRY in BasicButton

c890279

jobegrabber commented Feb 1, 2021

View reviewed changes

Remove leftover style tag

fd08106

jobegrabber removed request for noeljns and powlaa February 3, 2021 16:35

roschaefer approved these changes Feb 16, 2021

View reviewed changes

	:class="sidebarOpen ? '' : 'hidden'"
	:class="{sidebarOpen : 'hidden'}"

	<NavMenu :is-authenticated="isAuthenticated" @logout="logout()"></NavMenu>
	<NavMenu :is-authenticated="isAuthenticated" @logout="logout"></NavMenu>

		<style scoped>

		</style> No newline at end of file

Exercise8 #10

Are you sure you want to change the base?

Exercise8 #10

Uh oh!

Conversation

jobegrabber commented Feb 1, 2021

Uh oh!

jobegrabber left a comment

Choose a reason for hiding this comment

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!

jobegrabber commented Feb 16, 2021

Uh oh!

roschaefer commented Feb 16, 2021

Uh oh!

jobegrabber commented Feb 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants