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

feat: vue starter app #21

Merged
merged 30 commits into from
Oct 16, 2024
Merged

feat: vue starter app #21

merged 30 commits into from
Oct 16, 2024

Conversation

daine
Copy link
Collaborator

@daine daine commented Sep 4, 2024

Summary | Résumé

Adds a starter app for usage in Vue 3.

This starter app contains the following:

  • Regular JS (I'm hoping we can produce a typescript equivalent in the near future)
  • Vue Router
  • Pinia for state management
  • VI tests for unit tests
  • Playwright for E2E tests
  • ESLint configuration
  • Prettier configuration

@daine
Copy link
Collaborator Author

daine commented Sep 11, 2024

There is currently a dependency issue (security vulnerability) with send version 0.18.0 which I've traced to the serve-static package. Here is a PR, hoping it gets merged and published soon.

Copy link
Contributor

@melaniebmn melaniebmn left a comment

Choose a reason for hiding this comment

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

First round of review - I have a few files left that I will continue with tomorrow

starter-apps/vue/vue-template/index.html Outdated Show resolved Hide resolved
starter-apps/vue/vue-template/package.json Outdated Show resolved Hide resolved
starter-apps/vue/vue-template/src/App.vue Outdated Show resolved Hide resolved
starter-apps/vue/vue-template/src/App.vue Outdated Show resolved Hide resolved
starter-apps/vue/vue-template/src/views/About/About1.vue Outdated Show resolved Hide resolved
starter-apps/vue/vue-template/src/views/About/About1.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@ethanWallace ethanWallace left a comment

Choose a reason for hiding this comment

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

Just a couple comments/questions.

Do we plan to replace the lorem ipsum in the app content with other content?

@daine
Copy link
Collaborator Author

daine commented Sep 27, 2024

Updates since last review:

  • Removed font import

  • Renamed package name

  • Moved DateModified to each individual page

  • skip-to-href id updated to reflect correct id (main-content)

  • Replaced toggle slot with lang-toggle component, placed within AppLink

  • Added more props to Heading: marginTop and marginBottom

  • Added hint property to Input and Text Area

  • Added hint text to all inputs and text area on the ReportABug form

  • Added property error-message to the Input, but I didn't set it on the ReportABug form because it behaves a bit differently

  • Renamed "HomeView" to just "Home" since it's under the /views folder

  • Removed all added mb-400 on unnecessary Heading component

  • Rename "About1" to "Topic", completely dropped "About 2" since I don't think it was useful

  • Removed invalid link to non-existent About2 page

  • Replaced Lorem Ipsum with new English text

  • Added an error summary to the form

Did not do:

  • Rename HeaderBreadcrumbs - because it is a common pattern in Vue
  • On TopNav, remove alignment="right" - because it moved my nav items to the left :(
  • Add error-message to inputs on ReportABug form - its behaviour was a bit unexpected for me so we can talk about it
  • Export internal components into an index file - it may be an opinionated pattern so let's leave them to decide that on their own

Copy link
Contributor

@ethanWallace ethanWallace left a comment

Choose a reason for hiding this comment

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

Just one small thing on the report a bug page. Would you also be able to update the gcds package to the latest version?

import TextArea from '@/components/forms/TextArea.vue'
import Button from '@/components/forms/Button.vue'
import DateModified from '@/components/DateModified.vue'
import ErrorSummary from '@/components/forms/ErrorSummary.vue'
Copy link
Contributor

Choose a reason for hiding this comment

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

The ErrorSummary.vue file seems to be missing from components/forms.

ethanWallace
ethanWallace previously approved these changes Oct 3, 2024
Copy link
Contributor

@ethanWallace ethanWallace left a comment

Choose a reason for hiding this comment

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

LGTM!

I think we could do the content revision and French content in a new PR

Copy link
Contributor

@melaniebmn melaniebmn left a comment

Choose a reason for hiding this comment

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

Just a couple of small changes :)

starter-apps/vue/vue-template/index.html Outdated Show resolved Hide resolved
starter-apps/vue/vue-template/src/components/Header.vue Outdated Show resolved Hide resolved
starter-apps/vue/vue-template/src/components/Heading.vue Outdated Show resolved Hide resolved
starter-apps/vue/vue-template/package.json Outdated Show resolved Hide resolved
@daine daine dismissed melaniebmn’s stale review October 4, 2024 04:08

All request changes met and code has been updated

Copy link
Contributor

@melaniebmn melaniebmn left a comment

Choose a reason for hiding this comment

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

LGTM, let's ship it!

@daine daine merged commit ee1b8e6 into main Oct 16, 2024
2 checks passed
@daine daine deleted the feat/vue-starter-app branch October 16, 2024 21:38
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.

3 participants