-
Notifications
You must be signed in to change notification settings - Fork 44
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
Update frontend to Vue and rewrite backend #39
base: main
Are you sure you want to change the base?
Conversation
what is the status of that? What needs to be done to get it out of WIP? A simple bullet point list is enough 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have glanced the PR, and I am quite pushing for this to be approved, not in the "main" branch, but in a next
branch, where @whitewhidow can test and polish the branch before release
From what I have seen there is a lot of really needed work, the backend code split in files was sorely needed to keep this project maintainable
There are many details in this PR; first of all, only 2 commits please don't do that again, ever. This is so painful, and it's impossible for me (an outsider) to review the backend refactor, as it's monolithic
If the refactor was made with small commits, it could have been reviewed easily, as each commit can be read and tested individually
Random questions:
- removed script files (
windows-install.bat
), what are they supposed to be replaced with? - I see there are patches to node_modules, what is the reason behind?
@@ -1,179 +1,17 @@ | |||
**SideNoder** - A **cross platform sideloader** for Quest(1&2) standalone vr headset. | |||
# vue-sidenoder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file needs to be reverted, with the exception of new Preview 😉
@@ -1,21 +0,0 @@ | |||
MIT License |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
project needs a license file
@colthreepv one of the main differences is the way to two apps approach browsing: I'm working on a way to browse local files to also bring back that functionality. There are no individual commits going from this app to vue-sidenoder as that was written completely new from scratch and this MR is just a (now outdated) rebase. This also makes the front- and backend of both apps completely incompatible. I'm honestly not sure what the best approach would be to go forward about pulling some changes back into this repo without a full rewrite/rebase. |
In my opinion works indeed more smoothly than the original project, not requiring side installations Simple APK install or Folder APK install are already supported, and I think this covers base needs for users For local file browsing I would suggest a much simpler approach, just a switch toggle between remote/local. I think the decision rests on @whitewhidow, my suggestion is again to merge this PR on a Just here to push this useful project in one common repository, easier to keep updated, instead of 2 projects both with pros/cons |
Thanks for the rewrite @ov3rk1ll this looks and works really smoothly. I hope it gets merged somehow ! |
This MR contains the full rewrite/change based on 2db4aab from https://github.com/ov3rk1ll/vue-sidenoder.
Nearly all files are removed or at least modified so merging this would replace the entire code base.
As discussed in #30, I'll try to split this into multiple MRs but this is the entire code to make sure nothing is missed.