Skip to content

feat(Settings): Use new form components#3324

Merged
susnux merged 2 commits into
masterfrom
artonge/feat/use_new_settings_composent
Nov 19, 2025
Merged

feat(Settings): Use new form components#3324
susnux merged 2 commits into
masterfrom
artonge/feat/use_new_settings_composent

Conversation

@artonge
Copy link
Copy Markdown
Collaborator

@artonge artonge commented Nov 19, 2025 •
edited
Loading

Before After
image image

@kra-mo for validation as there were no mockups.
Part of nextcloud/server#55667

@artonge artonge self-assigned this Nov 19, 2025
@artonge artonge requested a review from susnux November 19, 2025 14:40
@artonge artonge added enhancement New feature or request 3. to review Waiting for reviews javascript Javascript related ticket labels Nov 19, 2025
@artonge artonge added this to the Nextcloud 33 milestone Nov 19, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 19, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@artonge artonge force-pushed the artonge/feat/use_new_settings_composent branch 2 times, most recently from ba8382e to bd65fad Compare November 19, 2025 15:18
@artonge
Copy link
Copy Markdown
Collaborator Author

artonge commented Nov 19, 2025

/compile /

@artonge artonge force-pushed the artonge/feat/use_new_settings_composent branch from bd65fad to a622270 Compare November 19, 2025 17:01
susnux
susnux reviewed Nov 19, 2025
View reviewed changes
Comment thread src/components/Settings/PhotosSourceLocationsSettings.vue
@@ -18,14 +18,16 @@
</li>
</div>

Copy link
Copy Markdown
Contributor

@susnux susnux Nov 19, 2025

Choose a reason for hiding this comment

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

Unrelated to the PR but the code above is invalid semantically (and for accessibility) as the li must be in an ol or ul element but is used within a div element.

Copy link
Copy Markdown
Collaborator Author

@artonge artonge Nov 19, 2025

Choose a reason for hiding this comment

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

#3326

susnux
susnux reviewed Nov 19, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

As this is using Vue 2 you need to set the legacy prop of the NcAppSettingsDialog to false, so here:

photos/src/components/Settings/SettingsDialog.vue

Lines 7 to 11 in cfd06cd

<NcAppSettingsDialog
:open="open"
:show-navigation="true"
:name="t('photos', 'Photos settings')"
@update:open="onClose">

add :legacy="false"

@artonge artonge force-pushed the artonge/feat/use_new_settings_composent branch from 31aeafb to 9bf6684 Compare November 19, 2025 18:04
@artonge
Copy link
Copy Markdown
Collaborator Author

artonge commented Nov 19, 2025

/compile /

@nextcloud-command
chore(assets): Recompile assets
854c638 
Signed-off-by: nextcloud-command <[email protected]>
@susnux susnux merged commit 8738d4e into master Nov 19, 2025
46 checks passed
@susnux susnux deleted the artonge/feat/use_new_settings_composent branch November 19, 2025 23:06
@kra-mo
Copy link
Copy Markdown
Member

kra-mo commented Nov 20, 2025

Nice, just a small comment:

For regular actions (like these two), I'd keep using a full-width NcButton instead of NcFormBoxButton.

NcFormBoxButton is only really meant to be used either for informational buttons where the action is secondary to the info they display, or for navigation.

I know this is a bit arbitrary and confusing, sorry! I'm thinking about it and I'll see if we can do something about it.

@kra-mo
Copy link
Copy Markdown
Member

kra-mo commented Nov 20, 2025 •
edited
Loading

…for the first one anyway as that is below a list. For the second one, it would probably be a better choice to turn it into a button with info anyway, merging it with the displayed folder above it. Look at Attachments folder in the Talk settings redesign as an example.

Then I'd keep using NcFormBoxButton, taking advantage of its features :)

@artonge
Copy link
Copy Markdown
Collaborator Author

artonge commented Nov 20, 2025

Thanks for the feedbacks @kra-mo.

Here is the follow-up: #3327

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

Reviewers

@susnux susnux susnux approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@CarlSchwan CarlSchwan CarlSchwan approved these changes

Assignees

@artonge artonge

Labels

3. to review Waiting for reviews enhancement New feature or request javascript Javascript related ticket

Projects

None yet

Milestone

Nextcloud 33

Development

Successfully merging this pull request may close these issues.

6 participants

@artonge @kra-mo @susnux @skjnldsv @CarlSchwan @nextcloud-command