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

Vue rewrite #81

Merged
merged 285 commits into from
Dec 15, 2023
Merged

Vue rewrite #81

merged 285 commits into from
Dec 15, 2023

Conversation

guyguy2001
Copy link
Contributor

Feel free to make change requests based on the current code, however, I am currently working on making everything comply with TS, removing redundant imports and dependencies, and adding vue-cli as a dependency.

Copy link
Member

@luxaritas luxaritas left a comment

Choose a reason for hiding this comment

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

Quick high level thing before going into more specific changes - there's a bunch of files that need to get dropped

  • git bundle files, bundle.bat
  • Entire public folder (assuming the Vue cli is handling all that now)
  • I think there's some left over files from the old version - chat.html, chat.js, and chat.css aren't used any more are they?

@luxaritas
Copy link
Member

Would be good to do a package upgrade too - remove package-lock.json, run npm-check-updates, and reinstall. I think there's going to be some additional errors with new versions of things though.

@luxaritas luxaritas self-requested a review June 12, 2019 20:41
luxaritas
luxaritas previously approved these changes Jun 12, 2019
@luxaritas luxaritas self-requested a review June 12, 2019 20:41
@luxaritas luxaritas dismissed their stale review June 12, 2019 20:42

Partial review

Copy link
Member

@luxaritas luxaritas left a comment

Choose a reason for hiding this comment

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

Looks great! Most stuff is just nitpicks.

General notes

There are a number of IDs on elements that aren't needed. The only case where you need an id on an element should be if you want to use a style on it, and even then I'd probably suggest a class instead.

Looks like these still need removing:

  • chat.html
  • chat.js
  • chat.css
  • bundle.bat

As we talked about, there needs to be a test page, and a way to instantiate the app itself on a page by passing in an element ID after including a script

In a number of places, you're using v-show when you really should be using v-if - v-show is mainly for things that will often be getting shown and hidden (I'd do it for, say, the minimization of the chat but not for the connection button/chat input)

For components with no slots, use self-closing tags. I think there's an eslint-vue setting for that. I'd also prefer PascalCase to differentiate them from HTML elements.

Rename *Tab to *Panel. TabsPanel then just becomes Tabs or maybe TabList. The tabs are the things that go across the top that you click on, the panels hold the content for specific tabs. I named them how you are in the old version, but in hindsight that was rather confusing.

.eslintignore Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
src/App.vue Outdated Show resolved Hide resolved
src/App.vue Outdated Show resolved Hide resolved
src/App.vue Outdated Show resolved Hide resolved
src/store/actions.ts Outdated Show resolved Hide resolved
src/tools/generateNick.ts Outdated Show resolved Hide resolved
src/tools/parseUsername.ts Outdated Show resolved Hide resolved
src/types/consts.ts Outdated Show resolved Hide resolved
src/types/modules/irc-framework/irc-framework.d.ts Outdated Show resolved Hide resolved
src/App.vue Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Sep 18, 2019

CLA assistant check
All committers have signed the CLA.

@guyguy2001
Copy link
Contributor Author

guyguy2001 commented Jan 3, 2020

Closes #9.
We need to test whether it also applies to #63.

@guyguy2001
Copy link
Contributor Author

Note - I need to add the EJS hook

@luxaritas
Copy link
Member

Reworked this such that the initial vue bootstrap was set as an orphan, and then merged together unrelated histories with -s ours. Wanted to rebase on top of latest dev, but couldnt get it to work reasonably.

That said, finally merging this into dev now in preparation to actually using this. Will make any further necessary updates in dev.

@luxaritas luxaritas merged commit 3adfed3 into dev Dec 15, 2023
1 check passed
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.

4 participants