Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create project scaffolding #481

Merged

Conversation

temitopeadesoji
Copy link
Contributor

@temitopeadesoji temitopeadesoji commented Nov 3, 2022

Changes

  • install Nuxt3 with TypeScript
  • Install and Setup Vuetify
  • Install and Setup Pinia
  • Added Vitest

closes #477

Notes

@temitopeadesoji temitopeadesoji added the WIP Work in Progress label Nov 3, 2022
@temitopeadesoji temitopeadesoji force-pushed the 477-create-project-scaffolding branch from 0ab6996 to a28c001 Compare November 17, 2022 14:31
@temitopeadesoji temitopeadesoji self-assigned this Nov 17, 2022
@temitopeadesoji temitopeadesoji removed the WIP Work in Progress label Nov 17, 2022
const resetCount = counterState.$reset.bind(counterState)
</script>

<style lang="scss">
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't these be scoped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used a BEM class naming convention, so there shouldn't be a need for adding scoped

Copy link
Member

Choose a reason for hiding this comment

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

That's totally true! But just to avoid unexpected errors in the future we might want to switch those

Copy link

Choose a reason for hiding this comment

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

I think scoped component styles are better than BEM conventions.

Same effect, but less developer discipline required and no chance of collision.

Copy link
Contributor

@ihardz ihardz Apr 1, 2023

Choose a reason for hiding this comment

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

@temitopeadesoji , does not matter what convetion you use for class naming. The problem is in styles isolation.

Solutions: In order to keep the app scalable, nested components should not affect the parent components with their styles

* Choosing to export testing library through a utility file instead to avoid major refactorings and allowing custom configuration.
* Please see: https://testing-library.com/docs/react-testing-library/setup as reference
*/
export * from '@testing-library/vue'
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this? As we are using Vitest I think we could delete this file and remove the library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am currently using it for referencing items in the DOM and also triggering dom events

Copy link

Choose a reason for hiding this comment

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

You still want to use @testing-library with vitest, they solve separate problems

@netlify
Copy link

netlify bot commented Jan 11, 2023

Deploy Preview for starter-dev ready!

Name Link
🔨 Latest commit eb248c9
🔍 Latest deploy log https://app.netlify.com/sites/starter-dev/deploys/642339623945a70009c57b33
😎 Deploy Preview https://deploy-preview-481--starter-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@temitopeadesoji temitopeadesoji force-pushed the 477-create-project-scaffolding branch from 91f7bec to 715d80b Compare January 11, 2023 13:35
@@ -0,0 +1,88 @@
import { describe, expect, test } from 'vitest'
import { createTestingPinia } from '@pinia/testing'
import { fireEvent, render, screen } from '../../test/utils'
Copy link
Member

@jesus4497 jesus4497 Feb 12, 2023

Choose a reason for hiding this comment

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

Lets change this import to an alias instead!

Suggested change
import { fireEvent, render, screen } from '../../test/utils'
import { fireEvent, render, screen } from '@/test/utils'

const resetCount = counterState.$reset.bind(counterState)
</script>

<style lang="scss">
Copy link
Member

Choose a reason for hiding this comment

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

That's totally true! But just to avoid unexpected errors in the future we might want to switch those

app.use(pinia);

// Support for vuetify in Storybook
app.use(Vuetify);
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we could follow this example from their docs: https://storybook.js.org/recipes/vuetify#register-vuetify-in-storybook

@jesus4497 jesus4497 requested a review from ihardz March 27, 2023 19:40
@jesus4497 jesus4497 changed the base branch from main to 643-add-nuxt3-kit-cli March 31, 2023 15:55
@jesus4497 jesus4497 changed the base branch from 643-add-nuxt3-kit-cli to main March 31, 2023 15:57
@jesus4497 jesus4497 changed the base branch from main to feat/nuxt3-pinia-vuetify-kit March 31, 2023 19:22
// Assertions
expect(countValueCoomponent.textContent).toBe('0');

await fireEvent.click(button);
Copy link

Choose a reason for hiding this comment

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

I believe the testing-library recommends using the user-event module for most user-interactions and falling back to fireEvent for things user-event can't do

Copy link
Member

@jesus4497 jesus4497 Apr 4, 2023

Choose a reason for hiding this comment

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

I added the dependency in the project, lets focus on changing this when working on issue #479 - I think we should merge this so we can unblock the rest of the tickets

const { data } = await useFetch<string>(
'https://api.starter.dev/hello?greeting=from This Dot Labs!'
);
message.value = data.value || 'Failed to fetch';
Copy link

Choose a reason for hiding this comment

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

Suggested change
message.value = data.value || 'Failed to fetch';
message.value = data.value ?? 'Failed to fetch';

Comment on lines 1 to 11
import { defineStore } from 'pinia';

export interface ICounterRootState {
counter: number;
}

export const useCounterStore = defineStore('counterStore', {
state: (): ICounterRootState => ({
counter: 0,
}),
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets try to follow official neming conventions. For examples see the Pinia team vision about names:
https://pinia.vuejs.org/core-concepts/state.html#usage-with-the-options-api
Benefit: It will help the developers who will be going to use the kit to easy understand namings and follow naming strategy.
I love dotnet/java interfaces naming style, but Typescript has other best practices and frontent-only devs will be confused with such naming for interfaces.

Suggested change
import { defineStore } from 'pinia';
export interface ICounterRootState {
counter: number;
}
export const useCounterStore = defineStore('counterStore', {
state: (): ICounterRootState => ({
counter: 0,
}),
});
import { defineStore } from 'pinia';
export interface CounterState {
counter: number;
}
export const useCounterStore = defineStore('counter', {
state: (): CounterState => ({
counter: 0,
}),
});

Comment on lines 12 to 24
"scripts": {
"build": "nuxt build",
"dev": "nuxt dev",
"generate": "nuxt generate",
"preview": "nuxt preview",
"postinstall": "nuxt prepare",
"lint:js": "eslint --ext \".js,.ts,.vue\" --ignore-path .gitignore .",
"lint:prettier": "prettier --check .",
"lint": "yarn lint:js && yarn lint:prettier",
"lintfix": "prettier --write --list-different . && yarn lint:js --fix",
"test": "vitest --run --reporter verbose --globals",
"test:watch": "vitest --reporter verbose --globals"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, the best practice is to have 'start' and 'test' scripts.
That's why these scripts has shorthand run command:
npm run start <> npm start
npm run test <> npm test

So, I would suggest to add 'start' script

Comment on lines 23 to 30
try {
const { data } = await useFetch<string>(
'https://api.starter.dev/hello?greeting=from This Dot Labs!'
);
message.value = data.value || 'Failed to fetch';
} catch (e) {
message.value = 'Failed to fetch';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Catch block will never be executed. Seems like useFetch does not reject the promise!
See: https://nuxt.com/docs/api/composables/use-fetch#example :

const { data, pending, error, refresh } = await useFetch('/api/auth/login', {
  onRequest({ request, options }) {
    // Set the request headers
    options.headers = options.headers || {}
    options.headers.authorization = '...'
  },
  onRequestError({ request, options, error }) {
    // Handle the request errors
  },
  onResponse({ request, response, options }) {
    // Process the response data
    return response._data
  },
  onResponseError({ request, response, options }) {
    // Handle the response errors
  }
})

Thus, either use interceptors or follow framework-specific conventions in this case for error handling: https://nuxt.com/docs/getting-started/error-handling

Copy link
Member

Choose a reason for hiding this comment

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

Yes, working on this on issue #478 - Will continue this feedback in there

Comment on lines 57 to 65
const increaseCount = () => {
counterState.counter++;
};

const decreaseCount = () => {
if (counterState.counter) {
counterState.counter--;
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

lets keep it simple

Suggested change
const increaseCount = () => {
counterState.counter++;
};
const decreaseCount = () => {
if (counterState.counter) {
counterState.counter--;
}
};
const increaseCount = () => counterState.counter++;
const decreaseCount = () => counterState.counter--;

Copy link
Contributor

Choose a reason for hiding this comment

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

and if needed, just make a button disabled

Copy link
Contributor

@ihardz ihardz left a comment

Choose a reason for hiding this comment

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

Please address the comments

"postinstall": "nuxt prepare",
"lint:js": "eslint --ext \".js,.ts,.vue\" --ignore-path .gitignore .",
"lint:prettier": "prettier --check .",
"lint": "yarn lint:js && yarn lint:prettier",
Copy link
Contributor

@ihardz ihardz Apr 1, 2023

Choose a reason for hiding this comment

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

Kits should work with npm/yarn/pnpm
Food for thoughts: Developer1 uses npm or pnpm and does't have yarn installed.

Copy link
Member

Choose a reason for hiding this comment

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

Added npm-run-all package in order to make this agnostic

Comment on lines 125 to 145
{
key: 'nuxt 3',
name: 'NuxtJS 3',
tags: ['Framework'],
Icon: (props) => <NuxtIcon {...props} />,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{
key: 'nuxt 3',
name: 'NuxtJS 3',
tags: ['Framework'],
Icon: (props) => <NuxtIcon {...props} />,
},
{
key: 'nuxt 3',
name: 'NuxtJS 3',
tags: ['Framework'],
Icon: (props) => <Nuxt3Icon {...props} />,
},

Comment on lines 1 to 37
import { Props } from './types';

export function NuxtIcon({ size = 48, className }: Props) {
return (
<svg
width={size}
height={size}
viewBox="0 0 46 46"
viewBox="0 0 900 900"
fill="none"
xmlns="http://www.w3.org/2000/svg"
className={className}
>
<g clipPath="url(#clip0_457_1244)">
<path
d="M14.2176 38.2971C14.1846 38.2388 14.1552 38.1786 14.1295 38.1167C13.9523 37.7063 13.8978 37.2532 13.9727 36.8125H3.47596L19.0754 9.36018L24.1968 18.3632L25.6814 15.7359L21.217 7.87136C21.0031 7.46858 20.6916 7.12584 20.3111 6.87448C19.9305 6.62313 19.493 6.47117 19.0386 6.4325C18.5851 6.45311 18.1465 6.60058 17.7727 6.85814C17.3988 7.1157 17.1048 7.473 16.924 7.8894L1.12061 35.6747C0.884391 36.067 0.748192 36.5113 0.723997 36.9686C0.699802 37.4259 0.788353 37.8821 0.98186 38.2971C1.22489 38.6804 1.57033 38.988 1.9791 39.1851C2.38787 39.3823 2.84365 39.4611 3.29489 39.4127H16.5313C16.08 39.4618 15.6239 39.3834 15.215 39.1861C14.8061 38.9889 14.4608 38.6809 14.2183 38.2971H14.2176Z"
fill="#00C492"
/>
<path
d="M44.6488 35.674L31.6623 12.7985C31.4493 12.3915 31.1371 12.0446 30.7547 11.79C30.3723 11.5354 29.9319 11.3812 29.4742 11.3416C29.0213 11.3613 28.583 11.5078 28.2093 11.7645C27.8357 12.0213 27.5417 12.3779 27.361 12.7937L25.6814 15.7352L27.1758 18.3632L29.4881 14.2693L42.3358 36.8118H37.4504C37.5128 37.1844 37.4777 37.5668 37.3484 37.9218C37.3211 38.0003 37.287 38.0762 37.2464 38.1487L37.2048 38.2319C36.9394 38.6068 36.5857 38.9105 36.175 39.1162C35.7643 39.3219 35.3093 39.4233 34.8501 39.4113H42.4926C42.9518 39.4236 43.407 39.3224 43.8177 39.1167C44.2285 38.911 44.5821 38.607 44.8472 38.2319C45.0565 37.8296 45.1489 37.3767 45.1138 36.9246C45.0787 36.4725 44.9177 36.0392 44.6488 35.674Z"
fill="#008776"
/>
<path
d="M37.2056 38.2327L37.2472 38.1495C37.2876 38.077 37.3215 38.0011 37.3485 37.9226C37.4778 37.5676 37.5129 37.1852 37.4505 36.8126C37.3784 36.4107 37.2341 36.0252 37.0245 35.6748L27.1855 18.3632L25.6821 15.7359L24.1877 18.3632L14.3521 35.6748C14.1612 36.0294 14.0328 36.4143 13.9726 36.8126C13.8935 37.2523 13.9433 37.7056 14.1163 38.1175C14.1419 38.1794 14.1713 38.2397 14.2044 38.2979C14.4475 38.6811 14.793 38.9885 15.2017 39.1856C15.6105 39.3826 16.0662 39.4613 16.5174 39.4128H34.8364C35.2978 39.4264 35.7554 39.3259 36.1686 39.1202C36.5818 38.9144 36.9378 38.6098 37.2049 38.2334L37.2056 38.2327ZM25.6814 20.9912L34.6747 36.8119H16.6922L25.6814 20.9912Z"
fill="#2D4A5D"
/>
</g>
<defs>
<clipPath id="clip0_457_1244">
<rect
width="45.4737"
height="45.4737"
fill="white"
transform="translate(0.263184 0.263153)"
/>
</clipPath>
</defs>
<path
d="M504.908 750H839.476C850.103 750.001 860.542 747.229 869.745 741.963C878.948 736.696 886.589 729.121 891.9 719.999C897.211 710.876 900.005 700.529 900 689.997C899.995 679.465 897.193 669.12 891.873 660.002L667.187 274.289C661.876 265.169 654.237 257.595 645.036 252.329C635.835 247.064 625.398 244.291 614.773 244.291C604.149 244.291 593.711 247.064 584.511 252.329C575.31 257.595 567.67 265.169 562.36 274.289L504.908 372.979L392.581 179.993C387.266 170.874 379.623 163.301 370.42 158.036C361.216 152.772 350.777 150 340.151 150C329.525 150 319.086 152.772 309.883 158.036C300.679 163.301 293.036 170.874 287.721 179.993L8.12649 660.002C2.80743 669.12 0.00462935 679.465 5.72978e-06 689.997C-0.00461789 700.529 2.78909 710.876 8.10015 719.999C13.4112 729.121 21.0523 736.696 30.255 741.963C39.4576 747.229 49.8973 750.001 60.524 750H270.538C353.748 750 415.112 713.775 457.336 643.101L559.849 467.145L614.757 372.979L779.547 655.834H559.849L504.908 750ZM267.114 655.737L120.551 655.704L340.249 278.586L449.87 467.145L376.474 593.175C348.433 639.03 316.577 655.737 267.114 655.737Z"
fill="#00DC82"
/>
</svg>
Copy link
Contributor

@ihardz ihardz Apr 1, 2023

Choose a reason for hiding this comment

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

Do not override existing icons. IF the newer framework version has different icon, Create new one!

Benefits: the main website visitor easy visually recognize different versions of appropriate technology

@jesus4497 jesus4497 force-pushed the 477-create-project-scaffolding branch from 6d96900 to 1c566bf Compare April 4, 2023 20:51
@jesus4497 jesus4497 requested review from ihardz and BigAB April 4, 2023 20:52
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@ihardz ihardz left a comment

Choose a reason for hiding this comment

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

Approved with known issues.
Comments to solve in next prs:

@jesus4497 jesus4497 merged commit 8b769c8 into feat/nuxt3-pinia-vuetify-kit Apr 5, 2023
@jesus4497 jesus4497 deleted the 477-create-project-scaffolding branch April 5, 2023 14:30
jdwilkin4 added a commit that referenced this pull request Jun 5, 2023
* Create project scaffolding (#481)

* Create project scaffolding

- install Nuxt3 with TypeScript
- Install and Setup Tailwind
- Install and Setup Pinia

* removed rule

* switch tailwind to vuetify

* updated git ignore

* add components and vuetify theme color

* fix linting

* added vitest tests

* added storybook

* chore: fix configs

* feat(wip): add storybook config

* chore: fix npm

* feat: add vuetify icon and packagejson config

* feat: update nuxt logo

* chore: remove storybook config

* fix: feedback comments

* feat: nuxt 3 icon

* feat: add start script

* fix: api example

* fix: stub fetch component

---------

Co-authored-by: jesus padron <[email protected]>

* [nuxt3-pinia-vuetify]: fix broken tests for counter example (#1217)

* fix: broken tests for counter example

* fix: pinning down dependencies

* Feat: Fetch api example (#1226)

* feat: fetch example logic

* fix: testing and files scripts order

* fix: removing duplicate express icon (#1231)

* [Nuxt 3 - Pinia - Vuetify] Feat: Update README  (#1229)

* feat: add readme file

* fix: typo

* fix: feedback

* fix: typos

* [Nuxt 3 - Pinia - Vuetify] Fix: sanity check (#1234)

* fix: sanity check

* fix: feedback

* Feat: add kit to cli (#1237)

* feat: add kit to cli

* feat: add kit to cli

* refactor: applying code review changes

* refactor: removing disabled attribute

* Update starters/nuxt3-pinia-vuetify/.nvmrc

Co-authored-by: Ihar Dziamidau <[email protected]>

* Update starters/nuxt3-pinia-vuetify/package.json

Co-authored-by: Ihar Dziamidau <[email protected]>

* Update starters/nuxt3-pinia-vuetify/components/FetchExample/FetchExample.vue

Co-authored-by: Ihar Dziamidau <[email protected]>

* refactor: removing optional text from fetch

* refactor: updating nuxt config

* chore: pinning package.json vue version

* fix: feedback from ihrar

---------

Co-authored-by: Temitope Adesoji <[email protected]>
Co-authored-by: jesus padron <[email protected]>
Co-authored-by: Ihar Dziamidau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Nuxt3-pinia-vuetify]: Create project scaffolding
4 participants