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

Port admin settings to vue #1880

Merged
merged 8 commits into from
Aug 30, 2022
Merged

Port admin settings to vue #1880

merged 8 commits into from
Aug 30, 2022

Conversation

CarlSchwan
Copy link
Member

No description provided.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan
Copy link
Member Author

Ended up also fixing the 25 compatibility issue as it was only a 4 lines change

package.json Outdated Show resolved Hide resolved
@Grotax
Copy link
Member

Grotax commented Aug 25, 2022

Hi thanks for working on this :)

Currently I can't get this running, I only see a white page in the admin settings.
The Browser console complains that it can't load "http://localhost:8080/apps/news/js/news-admin-settings.js?v=574f49ba-0"

I just ran make in the news directory and checked it with the builtin php server

@CarlSchwan
Copy link
Member Author

Hi thanks for working on this :)

Currently I can't get this running, I only see a white page in the admin settings. The Browser console complains that it can't load "http://localhost:8080/apps/news/js/news-admin-settings.js?v=574f49ba-0"

I just ran make in the news directory and checked it with the builtin php server

Should be fixed, I forgot to add it to the makefile

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
src/components/AdminSettings.vue Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
css/mobile.scss Outdated Show resolved Hide resolved
Co-authored-by: anoy. <anoymouserver@users.noreply.github.com>
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@Grotax
Copy link
Member

Grotax commented Aug 25, 2022

The build issue is solved 👍

While checking the settings I noticed that the top setting for useCronUpdates does not behave correctly.
By default it should be set to true: https://github.com/nextcloud/news/blob/master/lib/AppInfo/Application.php#L60

But in the UI the switch is set to false, if you then switch it to true it actually get's saved as false in the DB.
At least in sqlite I see the field saved as 0.

Co-authored-by: anoy. <anoymouserver@users.noreply.github.com>
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan CarlSchwan force-pushed the port/vue-settings branch 2 times, most recently from 006f179 to 3fd534b Compare August 25, 2022 12:31
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@Grotax
Copy link
Member

Grotax commented Aug 26, 2022

After quick test in my dev setup it seems to work now, changing the settings works as expected.

I will do more tests later.

@Grotax
Copy link
Member

Grotax commented Aug 26, 2022

I checked again, here are some things I noticed.

When changing a value in the admin settings there is no indicator if the change was saved or not, I think an indicator would be good.

Running make placed three files in the /js dir I think those should land in /js/build dir. Additionally they need to be copied like the other files to the sign dir, there is still a line to copy the old admin js.

@Grotax
Copy link
Member

Grotax commented Aug 26, 2022

The failing test is of course also a problem, I think the issue is that the cron setting useCronUpdates is now saved as 1/0 in the DB
While it used to be true/false. Could the UI use true/false booleans instead?

@Grotax
Copy link
Member

Grotax commented Aug 26, 2022

Oh turns out the app config does not support booleans so we need to deal with it :)

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.

I think with these changes it should work, tests run locally and I tested it in my dev instance and the toggle seems to behave like it should

lib/Service/StatusService.php Outdated Show resolved Hide resolved
tests/Unit/Service/StatusServiceTest.php Outdated Show resolved Hide resolved
Co-authored-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
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.

Looks good, only thing I'm missing is a visual indicator for successful saving :)

@CarlSchwan
Copy link
Member Author

Looks good, only thing I'm missing is a visual indicator for successful saving :)

Oh right, I forgot about it

@Grotax
Copy link
Member

Grotax commented Aug 27, 2022

I think the explore.scss can be deleted :)

@anoymouserver
Copy link
Contributor

While renaming the mobile.scss the 1024px instead of $breakpoint_mobile went missing again.

@CarlSchwan
Copy link
Member Author

While renaming the mobile.scss the 1024px instead of $breakpoint_mobile went missing again.

Fixed

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
Copy link
Contributor

@anoymouserver anoymouserver left a comment

Choose a reason for hiding this comment

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

The code looks good to me, I haven't tested it though.

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.

The success message is not popping up.

Error message works though.
Start php server load settings page, kill php server.
Change value -> Error Message 👍

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.

nvm, seems to be related to my dev setup.

I upgraded my test vm to NC25 and it worked, before that also tested on nc 24 also fine :)

@Grotax Grotax merged commit 753e887 into master Aug 30, 2022
@CarlSchwan
Copy link
Member Author

Thanks for merging :) I'll try to find some free time to port the main webui to vue but can't promise any ETA

@anoymouserver anoymouserver deleted the port/vue-settings branch August 31, 2022 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants