Rewrite with sidebase #41

DaeunYoon · 2022-09-05T23:28:11Z

closes nuxt/framework#38 & #12 & #39 & nuxt/framework#42

URL fixed (- instead space %20)
CSS package moved to Ant design
Inplemented suggestion endpoint with database (not used in frontend)
Getting mlm list from database
Updated README.md
Added footer & max width for contents

DaeunYoon · 2022-09-06T07:19:29Z

I changed <Head> and other SEO codes to useHead function.
I didn't write the test yet, but when I check on and the other tags did not recognize in the test.

-> useHead doesn't work for the test either. I'll try to figure this out :)

BracketJohn

please remove sidebase related stuff / replace it with old stuff where applicable (e.g.: main readme)

BracketJohn · 2022-09-06T07:26:26Z

.github/ISSUE_TEMPLATE/bug-report.yaml

@@ -0,0 +1,41 @@
+name: "\U0001F41E Bug report"


please remove these templates

BracketJohn · 2022-09-06T07:27:29Z

README.md

@@ -1,18 +1,65 @@
-# is-this-an-mlm


please:

keep the old readme

in the old readme already add a line like "made with sidebase" and then link sidebase

BracketJohn · 2022-09-06T07:28:21Z

app/pages/mlms/[id]/[name].vue

@@ -0,0 +1,29 @@
+<script setup lang="ts">
+import { mlms } from '../../../library/mlms'
+import TheMlm from '../../../components/TheMlm.vue'


You should be able to use ~ to shoerten these imports here

BracketJohn · 2022-09-06T07:28:54Z

app/server/api/example/[id].get.ts

@@ -0,0 +1,19 @@
+import { defineEventHandler } from 'h3'


please remove all unused example stuff -> this is an actual project made with sidebase, actual projects would also remove / replace everything that is from sidebase but not needed

BracketJohn · 2022-09-06T07:29:56Z

Do you have a specific question for this early review? (:

DaeunYoon · 2022-09-06T08:02:52Z

Do you have a specific question for this early review? (:

I wanted to make sure everything's going right with you as I thought I may take more time than you expected after wandering around the main branch.😅 Now on, I'll bring it up when there are more specific questions.

DaeunYoon · 2022-09-06T11:24:36Z

The web seems working fine but there are a few errors still on the testing parts. 1. `useHead` function and `Head` tags both are working on the web but give out errors on test.
I thought useHead can be used like the other composable but this work on the web but gives errors on test & ts error.
ReferenceError: useHead is not defined
Cannot find name 'useHead'.ts(2304)

(nuxt use-head example: https://v3.nuxtjs.org/examples/composables/use-head/)

For Head and other tags do not give ts errors but on test they also have errors.

[Vue warn]: Failed to resolve component: Meta If this is a native custom element, make sure to exclude it from component resolution via compilerOptions.isCustomElement.

@inkline/inkline library has error on test ( also works fine on the web)

-> moved ant design component

For this error, I couldn't figure out much yet but I'll keep on searching for both and will update here if I find something else that works.

…n/is-this-an-mlm into rewrite-with-sidebase

BracketJohn

For me the frontend does not work: "Company name" never disappears + there's an error in the console (dont know if this is related):

I use firefox on macos

the copy to clipboard button does not show up, but my browser supports it definitely!

BracketJohn · 2022-09-13T10:05:50Z

app/app.vue

+    <div class="w-full border-t text-center py-2 px-6 text-sm bg-slate-100 dark:bg-dark-lightgray">
+      <span class="dark:text-neutral-500 text-neutral-600">made with </span>
+      <a href="https://github.com/sidestream-tech/sidebase" class="text-SideBase-green" target="_blank">
+        SideBase


let's lowercase this to stay consistent with everywhere

BracketJohn · 2022-09-13T10:07:03Z

app/app.vue

+  </div>
+</template>
+
+<style>


please always use style sacoped where possible, see https://vue-loader.vuejs.org/guide/scoped-css.html - this way style does not pollute the global website.

please fix everywhere

I used scoped style in the other components but wanted this style to be a global style.
Which way is better to make global style if you recommend?

BracketJohn · 2022-09-13T10:11:46Z

app/server/api/company/[id].get.ts

+import { Company, companyFull } from '~/server/database/entities/Company'
+
+const paramsSchema = z.object({
+  id: z.string().regex(/^\d+$/).transform(Number),


nice handling to check whether id is really only numeric (:

BracketJohn · 2022-09-13T10:12:29Z

app/server/api/company/[id].get.ts

+  const params = parseParamsAs(event, paramsSchema)
+  return parseDataAs(Company.findOneOrThrow({ where: { id: params.id } }), companyFull)


there's two ways to improve this:

a)

Suggested change

const params = parseParamsAs(event, paramsSchema)

return parseDataAs(Company.findOneOrThrow({ where: { id: params.id } }), companyFull)

const where = parseParamsAs(event, paramsSchema)

return parseDataAs(Company.findOneOrThrow({ where }), companyFull)

b)

Suggested change

const params = parseParamsAs(event, paramsSchema)

return parseDataAs(Company.findOneOrThrow({ where: { id: params.id } }), companyFull)

const params = parseParamsAs(event, paramsSchema)

return parseDataAs(Company.findOneOrThrow({ where: params }), companyFull)

BracketJohn · 2022-09-13T10:12:59Z

app/server/api/suggestion/[name].post.ts

+  // const SUGGESTIONS_FOLDER = isProduction ? './suggestions' : '.'
+  // const filePath = `${SUGGESTIONS_FOLDER}/suggestions.log`
+  // fs.appendFileSync(filePath, `${now}; ${params.name}\n`)


I think this can go?

BracketJohn · 2022-09-13T10:13:26Z

app/server/api/suggestion/[name].post.ts

+
+  const payload = { name: params.name, date: now }
+
+  console.log(params.name)


and I think this can either go OR should be extended to be more informative, e.g., with a time?

BracketJohn · 2022-09-13T10:15:15Z

app/server/database/entities/Company.ts

+
+@Entity()
+export class Company extends Base {
+  @Column('text', { unique: true })


please do not add a ynique constraint here (unless you properly handle it in the endpoint and the frontend): many people may make similar or the same suggestions, and they should eithert all go through or the user should get good feedbackl like: "thanks, but this usggestion was already made!"

I would for now just allow all suggestions to keep the scope of this re-write on point. we can think about filtering out duplicates later on

Here, I made 2 entities. - Company (for saving mlms list) - Suggestion (for saving suggestions)
I thought as people can make non-make sense suggestions, adding suggestions directly to Company could be a bit dangerous. (though suggestion endpoint is not used in frontend yet)
What do you think about this?

For now, I removed the unique constraint from Suggestion and left it to Company.

We talked about this in daily! I changed it as you said! :)

DaeunYoon · 2022-09-14T09:01:00Z

[ ] the copy to clipboard button does not show up, but my browser supports it definitely!

I couldn't reproduce this error.
But sometimes it takes a bit of time to load copy button as isSupported comes along with useClipboard.
(but never been longer than a few seconds for me)
If this still happens to you, I can try

changing supported checking method
replace useClipboard with previous function

BracketJohn · 2022-09-19T08:59:02Z

I just re-tested and I also could not reproduce the error. So consider it fixed (:

BracketJohn

Almost there!

BracketJohn · 2022-09-19T08:59:58Z

README.md

@@ -15,4 +17,6 @@ Install `docker` and `docker-compose`. Then run:

 ## Development

-For development instructions, please have a look at the `README.md`s in each individual service directory.
+This website is build with [SideBase](https://github.com/sidestream-tech/sidebase) and following the basic stacks of SideBase.


Suggested change

This website is build with [SideBase](https://github.com/sidestream-tech/sidebase) and following the basic stacks of SideBase.

This website is build with [sidebase](https://github.com/sidestream-tech/sidebase) and following the basic stacks of sidebase.

Please user all lower case sidebase everywhere

BracketJohn · 2022-09-19T09:00:28Z

app/index.d.ts

@@ -0,0 +1 @@
+declare module 'fast-levenshtein';


is this still needed?

BracketJohn · 2022-09-19T09:01:05Z

app/package.json

+    "@vueuse/core": "^9.2.0",
+    "sqlite3": "^5.0.11",
+    "typeorm": "^0.3.9",
+    "zod": "^3.18.0"


this should not be needed anymore, you can import zod from nuxt-sidebase-parse!

zoey-kaiser

First of all, I really like the rewrite 😍! I feel like you did a great job using the tools provided by Nuxt3 to rewrite the app!

I have a few more questions outside of the review, that you could maybe help answer 🤗

Do you want to take a updated version of the scaffold and copy/paste your code into it?
Have you added a powered by sidebase reference yet (I could not find it)

zoey-kaiser · 2022-09-26T08:11:49Z

app/components/TheMlm.vue

+<script setup lang="ts">
+import type { PropType } from 'vue'
+import type { Mlm } from '../types'
+import TheShareGrid from './TheShareGrid.vue'


Do we want to use absolute imports here?

zoey-kaiser · 2022-09-26T08:12:44Z

app/components/TheMlm.vue

+      </h3>
+      <p class="h4 text-2xl">
+        Please check out
+        <nuxt-link to="https://en.wikipedia.org/wiki/Multi-level_marketing">


Do these open in a new tab? I personally prefer when external links do not move you away from our website, as this makes us lose user retention.

Sounds good! I updated it.

DaeunYoon · 2022-09-26T10:15:18Z

Thank you!

Do you want to take a updated version of the scaffold and copy/paste your code into it?

I'll have a look and if this wouldn't be an overkill I'll do that! :)

Have you added a powered by sidebase reference yet (I could not find it)

It's in the footer!

DaeunYoon · 2022-09-26T10:52:21Z

Opened new Issue #43 for updating version of the scaffold!

zoey-kaiser

Nit picks from now on 🤗

zoey-kaiser · 2022-09-28T13:45:56Z

app/components/TheMlm.vue

@@ -0,0 +1,59 @@
+<script setup lang="ts">
+import type { PropType } from 'vue'
+import TheShareGrid from './TheShareGrid.vue'


This still was not addressed. I meant we should change it to be:

Suggested change

import TheShareGrid from './TheShareGrid.vue'

import TheShareGrid from '~/components/TheShareGrid.vue'

This way, if we ever move the component, we won't have to reformat the imports (or at least it will be easier)

Edit: Only test and story files should use the relative imports, as these will always be in the same folder as the component.

zoey-kaiser · 2022-09-28T13:47:15Z

app/components/TheSearchBar.vue

+<script setup lang="ts">
+import type { PropType } from 'vue'
+import type { SelectValue } from 'ant-design-vue/es/select'
+import type { Mlm } from '../types'


Can we import this using

Suggested change

import type { Mlm } from '../types'

import type { Mlm } from '~/types'

zoey-kaiser · 2022-09-28T13:47:36Z

app/components/TheShareGrid.vue

+<script setup lang="ts">
+import { useClipboard } from '@vueuse/core'
+import type { PropType } from 'vue'
+import type { Mlm } from '../types'


zoey-kaiser · 2022-09-28T13:48:24Z

app/components/TheShareGrid.vue

+      >
+        <span v-if="!showIsCopied">Copy link</span>
+        <span v-else class="items-center justify-center" style="display: flex">
+          <CheckOutlined />


Do we want to use a margin here instead of the  ?

Suggested change

<CheckOutlined />

<CheckOutlined class="mr-1" />

zoey-kaiser · 2022-09-28T13:49:19Z

app/package.json

@@ -0,0 +1,68 @@
+{


Do we add some more infos about the project here? Like project name, github url, issue url etc.?

daeunYoon added 17 commits September 5, 2022 13:00

cloned sidebase

beae65f

cloned sidebase

f97de23

created button component

1632096

moved nuxt3 to scaffold

b4a0bdd

updated classes to tailwind

60d4cc1

update class name

d8981ad

added bottom line

22ddbd8

added margin

9e6282b

bottom line show up on all the pages

047b9f5

replaced url space to dash

7bae5e7

removed bottom line

84416ae

added resolver for inkline and tidy up the code

dbad8b8

fixed import

b47d3a7

changed to useHead

a8ee7a9

test

842afe8

fix lint and name page

b2c27ea

updated typo

713e404

BracketJohn requested changes Sep 6, 2022

View reviewed changes

daeunYoon added 3 commits September 6, 2022 11:27

updated readme

9d14a62

addressed changes

cf12794

removed redundant code

cf14de1

daeunYoon added 5 commits September 6, 2022 13:50

removed example code

4284e78

removed unsused code

2887dac

suggestion endpoint

14b3d85

removed unused code

c3b71e1

adjust with sidebase server update

c5c1609

daeunYoon and others added 9 commits September 13, 2022 10:21

updated test

7359ee8

fixed lint

007774b

updated test

73112a2

updates readme

db24a94

Update README.md

c3e36f0

updated suggestion entity

1e15a2d

Merge branch 'rewrite-with-sidebase' of https://github.com/BracketJoh…

1f89ff8

…n/is-this-an-mlm into rewrite-with-sidebase

changed folder name library to data

a78c543

seachbar loading state

8d8b3fe

BracketJohn requested changes Sep 13, 2022

View reviewed changes

daeunYoon added 2 commits September 13, 2022 14:28

addressing the change request

3b36ab0

updated searchbar loading state

5c038a3

DaeunYoon mentioned this pull request Sep 13, 2022

Error with useRoute, useRouter sidebase/sidebase#28

Closed

daeunYoon added 3 commits September 14, 2022 10:16

remove vue-router auto import

9a29c78

add button hover

d1df978

removed auto-imports/components

fdca077

BracketJohn reviewed Sep 19, 2022

View reviewed changes

daeunYoon added 3 commits September 19, 2022 11:22

adressing the changes

f5319a1

updated lint

c42e2cf

updated load fail message

b1ef39d

zoey-kaiser suggested changes Sep 26, 2022

View reviewed changes

daeunYoon added 2 commits September 26, 2022 12:22

addressed changes

3959664

updated sanp

10ca52c

zoey-kaiser suggested changes Sep 28, 2022

View reviewed changes

daeunYoon added 2 commits October 4, 2022 09:20

addressing the changes

50a18ba

fixed eslint

6575bc8

		const params = parseParamsAs(event, paramsSchema)
		return parseDataAs(Company.findOneOrThrow({ where: { id: params.id } }), companyFull)


		const payload = { name: params.name, date: now }

		console.log(params.name)

	This website is build with [SideBase](https://github.com/sidestream-tech/sidebase) and following the basic stacks of SideBase.
	This website is build with [sidebase](https://github.com/sidestream-tech/sidebase) and following the basic stacks of sidebase.

	import TheShareGrid from './TheShareGrid.vue'
	import TheShareGrid from '~/components/TheShareGrid.vue'

	import type { Mlm } from '../types'
	import type { Mlm } from '~/types'

Rewrite with sidebase #41

Are you sure you want to change the base?

Rewrite with sidebase #41

Conversation

DaeunYoon commented Sep 5, 2022 • edited Loading

DaeunYoon commented Sep 6, 2022 • edited Loading

BracketJohn 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

BracketJohn commented Sep 6, 2022

DaeunYoon commented Sep 6, 2022 • edited Loading

DaeunYoon commented Sep 6, 2022 • edited Loading

BracketJohn left a comment • edited by DaeunYoon Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaeunYoon Sep 13, 2022 • 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

DaeunYoon Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

DaeunYoon commented Sep 14, 2022

BracketJohn commented Sep 19, 2022

BracketJohn 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

zoey-kaiser 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

DaeunYoon commented Sep 26, 2022

DaeunYoon commented Sep 26, 2022

zoey-kaiser 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

DaeunYoon commented Sep 5, 2022 •

edited

Loading

DaeunYoon commented Sep 6, 2022 •

edited

Loading

DaeunYoon commented Sep 6, 2022 •

edited

Loading

DaeunYoon commented Sep 6, 2022 •

edited

Loading

BracketJohn left a comment •

edited by DaeunYoon

Loading

DaeunYoon Sep 13, 2022 •

edited

Loading

DaeunYoon Sep 14, 2022 •

edited

Loading