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.

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

enh(settings): Migrate admin settings for sharing to vue #41581

Merged
merged 3 commits into from
Nov 20, 2023
Merged

enh(settings): Migrate admin settings for sharing to vue #41581

merged 3 commits into from
Nov 20, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Nov 17, 2023 •
edited
Loading

Summary

Migrate sharing settings to get rid of select2 group select. Needed for accessibility.
Also fixes the not working "exclude groups from sharing" select.

Screenshots

before after
Screen Shot 2023-11-17 at 17 06 01 Screen Shot 2023-11-17 at 17 04 13
Screen Shot 2023-11-17 at 17 05 37 Screen Shot 2023-11-17 at 17 04 57

Screen cast

vokoscreenNG-2023-11-17_17-29-26.mp4

Checklist

@susnux susnux added this to the Nextcloud 28 milestone Nov 17, 2023
@susnux susnux requested review from JuliaKirschenheuter, nfebe, a team, sorbaugh, emoral435 and Pytal and removed request for a team November 17, 2023 16:02
@susnux susnux force-pushed the fix/migrate-admin-settings-sharing-vue branch 3 times, most recently from f8e8064 to 439e03c Compare November 17, 2023 16:29
JuliaKirschenheuter
JuliaKirschenheuter approved these changes Nov 17, 2023
View reviewed changes
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

🥳 very nice!

@susnux susnux force-pushed the fix/migrate-admin-settings-sharing-vue branch from 439e03c to e5800f6 Compare November 17, 2023 18:23
@susnux
Copy link
Contributor Author

susnux commented Nov 17, 2023

/compile amend/

@ShGKme
Copy link
Contributor

ShGKme commented Nov 17, 2023

There is an issue with textarea:
image

I see a different state sometimes, not sure if it is an issue.

master pr
image image

@susnux
Copy link
Contributor Author

susnux commented Nov 17, 2023

There is an issue with textarea:

Will be fixed with the nextcloud-vue update

I see a different state sometimes, not sure if it is an issue.

Thats because there is no such setting as disclaimer "disabled", before it just checks whether the disclaimer is falsy or truthy.
To distinguish I used following logic: If it is undefined then it is disabled, otherwise enabled.
It will be undefined if the setting is deleted on server, if you enable an empty string will be set on server.

@susnux susnux force-pushed the fix/migrate-admin-settings-sharing-vue branch from e5800f6 to 92c351b Compare November 17, 2023 23:09
@susnux susnux enabled auto-merge November 17, 2023 23:09
@ShGKme
Copy link
Contributor

ShGKme commented Nov 20, 2023

Dron error is unrelated

@blizzz blizzz mentioned this pull request Nov 20, 2023
Merged
5 tasks
@susnux
enh(settings): Migrate admin settings for sharing to vue
22163c6 
This is required to get the fixes for a11y from `@nextcloud/vue`.

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux
fix(config): Adjust webpack config to provide appName and `appVersi…
8fc0395 
…on` which are expected by `@nextcloud/vue`

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux
chore: Compile assets
543e2db 
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the fix/migrate-admin-settings-sharing-vue branch from 92c351b to 543e2db Compare November 20, 2023 16:10
@susnux susnux disabled auto-merge November 20, 2023 16:31
@susnux susnux merged commit 02807a3 into master Nov 20, 2023
@susnux susnux deleted the fix/migrate-admin-settings-sharing-vue branch November 20, 2023 16:32
@come-nc
Copy link
Contributor

come-nc commented Nov 21, 2023

Drone error very related, this broke tests for master:

452	There were 4 errors:
453	
454	1) OCA\Settings\Tests\Settings\Admin\SharingTest::testGetFormWithoutExcludedGroups
455	ArgumentCountError: Too few arguments to function OCA\Settings\Settings\Admin\Sharing::__construct(), 4 passed in /drone/src/apps/settings/tests/Settings/Admin/SharingTest.php on line 68 and exactly 7 expected
456	
457	/drone/src/apps/settings/lib/Settings/Admin/Sharing.php:48
458	/drone/src/apps/settings/tests/Settings/Admin/SharingTest.php:68
459	
460	2) OCA\Settings\Tests\Settings\Admin\SharingTest::testGetFormWithExcludedGroups
461	ArgumentCountError: Too few arguments to function OCA\Settings\Settings\Admin\Sharing::__construct(), 4 passed in /drone/src/apps/settings/tests/Settings/Admin/SharingTest.php on line 68 and exactly 7 expected
462	
463	/drone/src/apps/settings/lib/Settings/Admin/Sharing.php:48
464	/drone/src/apps/settings/tests/Settings/Admin/SharingTest.php:68
465	
466	3) OCA\Settings\Tests\Settings\Admin\SharingTest::testGetSection
467	ArgumentCountError: Too few arguments to function OCA\Settings\Settings\Admin\Sharing::__construct(), 4 passed in /drone/src/apps/settings/tests/Settings/Admin/SharingTest.php on line 68 and exactly 7 expected
468	
469	/drone/src/apps/settings/lib/Settings/Admin/Sharing.php:48
470	/drone/src/apps/settings/tests/Settings/Admin/SharingTest.php:68
471	
472	4) OCA\Settings\Tests\Settings\Admin\SharingTest::testGetPriority
473	ArgumentCountError: Too few arguments to function OCA\Settings\Settings\Admin\Sharing::__construct(), 4 passed in /drone/src/apps/settings/tests/Settings/Admin/SharingTest.php on line 68 and exactly 7 expected
474	
475	/drone/src/apps/settings/lib/Settings/Admin/Sharing.php:48
476	/drone/src/apps/settings/tests/Settings/Admin/SharingTest.php:68

@nickvergessen
Copy link
Member

Please update the documentation screenshots:
https://docs.nextcloud.com/server/latest/admin_manual/configuration_files/file_sharing_configuration.html

@nickvergessen nickvergessen added the pending documentation This pull request needs an associated documentation update label Nov 21, 2023
@artonge artonge mentioned this pull request Nov 21, 2023
Merged
@artonge
Copy link
Contributor

artonge commented Nov 21, 2023

Fix attempt here: #41637

@susnux susnux mentioned this pull request Nov 22, 2023
Merged
@susnux
Copy link
Contributor Author

susnux commented Nov 22, 2023

Please update the documentation screenshots

Done nextcloud/documentation#11300

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JuliaKirschenheuter JuliaKirschenheuter JuliaKirschenheuter approved these changes

@Pytal Pytal Pytal approved these changes

@ShGKme ShGKme ShGKme approved these changes

@nfebe nfebe Awaiting requested review from nfebe

@sorbaugh sorbaugh Awaiting requested review from sorbaugh sorbaugh was automatically assigned from nextcloud/server-frontend

@emoral435 emoral435 Awaiting requested review from emoral435 emoral435 was automatically assigned from nextcloud/server-frontend

Assignees

@susnux susnux

Labels
3. to review Waiting for reviews accessibility bug feature: settings pending documentation This pull request needs an associated documentation update
Projects
None yet
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.

[BITV] Replace all places with old NcMultiselect components with NcSelect components
7 participants
@susnux @ShGKme @come-nc @nickvergessen @artonge @JuliaKirschenheuter @Pytal