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

fix: Use nextcloud-vue components for personal info settings #43488

Merged
merged 4 commits into from
Feb 15, 2024
Merged

fix: Use nextcloud-vue components for personal info settings #43488

merged 4 commits into from
Feb 15, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Feb 9, 2024

Summary

Use our common styles to make the look consistent.

Checklist

@susnux susnux added bug design Design, UI, UX, etc. 3. to review Waiting for reviews feature: settings labels Feb 9, 2024
@susnux susnux added this to the Nextcloud 29 milestone Feb 9, 2024
ShGKme

This comment was marked as resolved.

Sign in to view

@ShGKme

This comment was marked as resolved.

Sign in to view
@susnux

This comment was marked as resolved.

Sign in to view
@susnux susnux added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 9, 2024
@susnux susnux force-pushed the fix/use-nc-components-account-property-section branch from ee48e44 to 3894f4a Compare February 10, 2024 18:57
@susnux susnux added 3. to review Waiting for reviews and removed backport-request 2. developing Work in progress labels Feb 10, 2024
@susnux susnux requested a review from ShGKme February 10, 2024 18:58
@susnux
Copy link
Contributor Author

susnux commented Feb 10, 2024

Resolved the issue with different input sizes. Needed to refactor all sections to use nextcloud vue inputs. Moreover some other minor issues are now fixed.

And everything is CI tested now.

szaimen
szaimen approved these changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested and seems to work. Also looks amazing 🤩
(but didnt review the code)

@szaimen
Copy link
Contributor

szaimen commented Feb 13, 2024 •
edited
Loading

Only small thing I noticed is that such a phone number seems to be invalid but it should be valid?
image
(apparently the country code, e.g. +49 instead of 0 is required but it should not be?)

@susnux
Copy link
Contributor Author

susnux commented Feb 13, 2024

@szaimen I did not change anything on the logic there - I just show an error when there is one. Before it silently failed and did not save anything.
So it now at least shows that there is something not right and will not save it.

(libphonenumber is the dependency we use there).

@szaimen
Copy link
Contributor

szaimen commented Feb 13, 2024

@szaimen I did not change anything on the logic there - I just show an error when there is one. Before it silently failed and did not save anything. So it now at least shows that there is something not right and will not save it.

(libphonenumber is the dependency we use there).

Ah I see. Fine then I guess 👍

Pytal
Pytal approved these changes Feb 14, 2024
View reviewed changes
Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

Other than minor remarks, code looks good!

apps/settings/src/components/PersonalInfo/EmailSection/Email.vue Outdated Show resolved Hide resolved
apps/settings/src/components/PersonalInfo/shared/AccountPropertySection.vue Outdated Show resolved Hide resolved
emoral435
emoral435 approved these changes Feb 14, 2024
View reviewed changes
Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

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

LGTM! Great PR :)

apps/settings/src/components/PersonalInfo/EmailSection/Email.vue
Comment on lines +40 to +42
<NcActions :aria-label="actionsLabel" @close="showFederationSettings = false">
<template v-if="showFederationSettings">
<NcActionButton @click="showFederationSettings = false">
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Isn't the latter @click redundant, due to the parent NcActions having the same effect on close? It can be removed, because the action gets done twice whenever the button is clicked to close.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No thats two different actions as this is a "submenu".
This one (the button click) is to allow the user to go back in the menu. The close event is to reset the menu to the main menu on close.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. My bad!

susnux and others added 2 commits February 15, 2024 00:46
@susnux
fix(settings): Migrate AccountPropertySection to @nextcloud/vue com…
fe58d8a 
…ponents

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux @Pytal
fix(settings): Use status states from NcInputField instead of custo…
3e09295 
…m handling

Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de>
Co-authored-by: Pytal <24800714+Pytal@users.noreply.github.com>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
enh(settings): Add Cypress tests for personal information
e292cf5 
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
chore: Compile assets
274867f 
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the fix/use-nc-components-account-property-section branch from 8eaf83c to 274867f Compare February 14, 2024 23:54
@susnux susnux dismissed ShGKme’s stale review February 15, 2024 02:14

changes implemented

@susnux susnux merged commit 8134559 into master Feb 15, 2024
97 checks passed
@susnux susnux deleted the fix/use-nc-components-account-property-section branch February 15, 2024 02:15
ShGKme
ShGKme reviewed Feb 28, 2024
View reviewed changes
apps/settings/src/constants/AccountPropertyConstants.js Show resolved Hide resolved
apps/settings/src/components/PersonalInfo/LocaleSection/Locale.vue Show resolved Hide resolved
apps/settings/src/components/PersonalInfo/LocaleSection/Locale.vue Show resolved Hide resolved
apps/settings/src/components/PersonalInfo/LanguageSection/LanguageSection.vue Show resolved Hide resolved
apps/settings/src/components/PersonalInfo/shared/FederationControl.vue
Comment on lines +33 to +38
<FederationControlActions :additional="additional"
:additional-value="additionalValue"
:handle-additional-scope-change="handleAdditionalScopeChange"
:readable="readable"
:scope="scope"
@update:scope="onUpdateScope" />
Copy link
Contributor

Choose a reason for hiding this comment

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

NcActions doesn't fully support having NcAction* as non-direct children. It breaks a11y fixes for this component from #43098

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like fixing in components? Can we not provide some registration in actions and inject in NcAction* component to register the different types? So we know for sure which aria role to assign?

@ShGKme ShGKme mentioned this pull request Mar 1, 2024
Merged
4 tasks
@blizzz blizzz mentioned this pull request Mar 5, 2024
Merged
@Antreesy Antreesy mentioned this pull request Oct 9, 2024
Draft
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pytal Pytal Pytal approved these changes

@ShGKme ShGKme ShGKme left review comments

@emoral435 emoral435 emoral435 approved these changes

@szaimen szaimen szaimen approved these changes

@JuliaKirschenheuter JuliaKirschenheuter Awaiting requested review from JuliaKirschenheuter

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug design Design, UI, UX, etc. feature: settings
Projects
None yet
Milestone
Nextcloud 29
Development

Successfully merging this pull request may close these issues.

[Bug]: Disabled HTML inputs don't appear disabled visually
5 participants
@susnux @ShGKme @szaimen @Pytal @emoral435