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

Settings: reduce amount of sections #9879

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Conversation

GVodyanov
Copy link
Contributor

Fix #9837

Account creation: account creation
Appearance and accessibility: layout, activate body search, gravatar settings, reply text position, sorting, keyboard shortcuts
Privacy and security: data collection consent, trusted senders, S/MIME, mailvelope
Assistance features: assistance features
Mailto: mailto

A B
image Screenshot from 2024-07-17 15-51-25

Also:

  • renamed "Keyboard" to "Keyboard shortcuts"
  • added an h6 tag to describe every setting (h6 because anything larger would be larger than the 'NcAppSettingsSection` title)

@ChristophWurst
Copy link
Member

added an h6 tag to describe every setting (h6 because anything larger would be larger than the 'NcAppSettingsSection` title)

use the heading that fits semantically. we have to fix/adjust the font size instead. is the heading too small?

@GVodyanov
Copy link
Contributor Author

added an h6 tag to describe every setting (h6 because anything larger would be larger than the 'NcAppSettingsSection` title)

use the heading that fits semantically. we have to fix/adjust the font size instead. is the heading too small?

image

Here's what it looks like when it's "correct" (an h4 seeing as the NcAppSettingsSection is an h3)... Gonna make a PR in nextcloud vue ig

Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Nice! Way easier to understand now. Agreed about the heading sizes, we should use the semantically correct heading tag. Some additional comments related to mostly wording:

  • Wording changes:
    • "Appearance and accessibility" as a heading may be misleading as it contains settings related to composing, searching, etc. We could just use the heading "General" (ref Gmail)
    • "Auto tag text" --> "Mark as important"
    • "Activate body search" --> "Search in body"
    • "Privacy and Security" --> "Privacy and security" (no capital S)
  • "Keyboard shortcuts" should be its own category as that's more accessible, it can be the last category
  • "Mailto" as a separate item seems a bit strange, it can just go into the general settings now that it's not related to appearance
  • Order of general settings:
    • Layout
    • Sorting,
    • Search in body
    • Reply text position
    • Gravatar
    • Mailto

@ChristophWurst
Copy link
Member

We out keyboard shortcuts into Appearance and accessibility because server has it there too.

@GVodyanov
Copy link
Contributor Author

GVodyanov commented Jul 19, 2024

Nice! Way easier to understand now. Agreed about the heading sizes, we should use the semantically correct heading tag. Some additional comments related to mostly wording:

* [x]  Wording changes:
  
  * [x]  "Appearance and accessibility" as a heading may be misleading as it contains settings related to composing, searching, etc. We could just use the heading "General" (ref Gmail)
  * [ ]  "Auto tag text" --> "Mark as important"
  * [x]  "Activate body search" --> "Search in body"
  * [x]  "Privacy and Security" --> "Privacy and security" (no capital S)

* [x]  "Keyboard shortcuts" should be its own category as that's more accessible, it can be the last category

* [x]  "Mailto" as a separate item seems a bit strange, it can just go into the general settings now that it's not related to appearance

* [x]  Order of general settings:
  
  * [x]  Layout
  * [x]  Sorting,
  * [x]  Search in body
  * [x]  Reply text position
  * [x]  Gravatar
  * [x]  Mailto

@nimishavijay Done, what do you mean by "auto tag text" though?

EDIT: now that Christoph explained, changed that too :)

@ChristophWurst
Copy link
Member

what do you mean by "auto tag text" though?

Automatically classify importance of new email

@hamza221
Copy link
Contributor

I think it was closed by mistake

@hamza221 hamza221 reopened this Jul 19, 2024
@GVodyanov
Copy link
Contributor Author

Hey @nimishavijay does this look alright?

@ChristophWurst
Copy link
Member

@nimishavijay's suggestions are addressed. Let's get this in 👍

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

:shipit:

Signed-off-by: Grigory V <scratchx@gmx.com>
@GVodyanov GVodyanov force-pushed the style/reduce-settings-sections branch from 6d39309 to 1118fa0 Compare July 25, 2024 08:13
@GVodyanov GVodyanov enabled auto-merge (squash) July 25, 2024 08:13
@GVodyanov GVodyanov merged commit 845b918 into main Jul 25, 2024
27 checks passed
@GVodyanov GVodyanov deleted the style/reduce-settings-sections branch July 25, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rearrange app settings sections
4 participants