Skip to content

feat: vue starter app #21

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.

Sign up for GitHub

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

Jump to bottom
Merged
daine merged 30 commits into from
Oct 16, 2024
Merged

feat: vue starter app #21

daine merged 30 commits into from
Oct 16, 2024

Conversation

@daine
Copy link
Collaborator

@daine daine commented Sep 4, 2024 •
edited
Loading

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 •
edited
Loading

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.

melaniebmn
melaniebmn requested changes Sep 12, 2024
View reviewed changes
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

@daine daine requested a review from ethanWallace September 18, 2024 07:49
ethanWallace
ethanWallace requested changes Sep 18, 2024
View reviewed changes
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

ethanWallace
ethanWallace requested changes Oct 2, 2024
View reviewed changes
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?

starter-apps/vue/vue-template/src/views/ReportABug.vue
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

@ethanWallace ethanWallace Oct 2, 2024

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 mentioned this pull request Oct 2, 2024
Merged
@daine daine requested a review from ethanWallace October 3, 2024 01:40
ethanWallace
ethanWallace previously approved these changes Oct 3, 2024
View reviewed changes
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

melaniebmn
melaniebmn previously requested changes Oct 3, 2024
View reviewed changes
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 :)

@daine daine dismissed melaniebmn’s stale review October 4, 2024 04:08

All request changes met and code has been updated

daine
daine commented Oct 4, 2024
View reviewed changes
melaniebmn
melaniebmn approved these changes Oct 16, 2024
View reviewed changes
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
@daine daine deleted the feat/vue-starter-app branch October 16, 2024 21:38
delisma pushed a commit to gc-proto/gcds-examples that referenced this pull request Aug 19, 2025
@daine
feat: vue starter app (cds-snc#21)
b29e04e 
* chore: add wip pieces

* chore: add routing and i18n

* chore: add routing, breadcrumbs, 404

* add tests and updated bug report form

* chore: add tests

* chore: add intro text

* chore: remove typescript - not in definition of done

* chore: remove logo

* chore: cleanup

* chore: internal components + updated tests

* chore: change p to text

* chore: change favicon

* fix: deps

* chore: update playwright

* fix: deps package lock

* chore: update deps

* docs: update readme

* chore: PR feedback changes

* chore: deps update, put back alignment=right on top nav

* chore: PR feedback

* chore: add text instead of lorem ipsum

* chore: npm audit fix

* chore: missed one file

* chore: update package versions

* chore: PR review changes

* chore: update comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@melaniebmn melaniebmn melaniebmn approved these changes

@ethanWallace ethanWallace Awaiting requested review from ethanWallace

Assignees

@melaniebmn melaniebmn

@ethanWallace ethanWallace

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@daine @melaniebmn @ethanWallace