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] Upmerge Recent Changes #1982

Merged
merged 5 commits into from
Nov 17, 2022

Conversation

devlinjunker
Copy link
Contributor

@devlinjunker devlinjunker commented Nov 10, 2022

NOTE: Please Review this PR carefully... lots of changes!

Related to #748 and #1880

Follows #1968

Upmerge Recent Changes from master

  • API Changes
  • Admin Rewrite as vue component
  • Integration tests (should work... will test with this PR)

Testing Completed

  • Verified that App Builds with make

  • Verified that Admin Settings Page Loads on localhost (NC 25)
    admin

  • Verified that App loads on localhost (NC 25)
    vue app

  • Verified that Jest Unit Tests Pass
    tests

Up Next

Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
Copy link
Member

@Grotax Grotax left a comment

Choose a reason for hiding this comment

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

Had a look on the changes and looks ok.
Most changes affect the language files which can be ignored.

I think the main review needs to happen once we merge the vue rewrite into master

@Grotax
Copy link
Member

Grotax commented Nov 10, 2022

ah the tests should be fixed of course :)

Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
@devlinjunker
Copy link
Contributor Author

hmm... strange.. I had to use node version 18 to update the package-lock file although the workflows seem to only be using 16

I'm noticing a lot of the translations were removed in this PR (so removed from master branch?)... @Grotax do you know why that was?

@Grotax
Copy link
Member

Grotax commented Nov 10, 2022

Translations are not managed by this repository so we automatically receive whatever translations are created.
When we change the original English text Somone might change it.
If the translation was bad it might get deleted and so on...

I don't monitor it

@devlinjunker
Copy link
Contributor Author

devlinjunker commented Nov 10, 2022

oh alright, sounds good then

any idea what's going on with these integration tests for php 8.2? https://github.com/nextcloud/news/actions/runs/3438626164/jobs/5735094897

I'll clean up the linting errors tonight

@anoymouserver
Copy link
Contributor

You can ignore the PHP 8.2 tests. NC does not support it yet, but it might be as soon as there is an official 8.2 release later this year.

Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
@devlinjunker
Copy link
Contributor Author

I didn't verify the linting fixes last night before pushing but I did this morning and they should be working now

sorry about that!

@devlinjunker
Copy link
Contributor Author

seems like we're good to merge this one now and I'll start on the next steps in this PR

@Grotax Grotax merged commit 095176f into nextcloud:vue-rewrite Nov 17, 2022
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.

None yet

3 participants