Correct text and icon alignments in server audit dialog #10925

banobepascal · 2025-01-14T00:39:09Z

Change-Id: Id4b45e9097b8ba77df5280bedf1635693fce72fc

Resolves: Audit dialog has unwanted grey dot icons and wrong text alignment #10841
Target version: master

Summary

Improvements in alignment of icons and text in the server audit dialog

IN (COOLWSD version: 24.04.8.1(git hash: `8475197` (E))

AFTER: #10870

CURRENT FIX AND IMPROVEMENT

Checklist

I have run make prettier-write and formatted the code.
All commits have Change-Id
I have run tests with make check
I have issued make run and manually verified that everything looks okay
Documentation (manuals or wiki) has been updated or is not required

banobepascal · 2025-01-14T08:15:07Z

@pedropintosilva please review, and I would also like your feedback on "No issues found" styling

banobepascal · 2025-01-20T07:34:12Z

@pedropintosilva advise on the darkmode icon.

pedropintosilva · 2025-01-20T09:38:06Z

Nice, Maybe the indicator could look more line one element (distinct from the others rows and compound by icon and text) if the text would be set with the same dark green as the outer circle of the new icon. Another option would be to keep the text black but set a subtle green background that encompasses both the icon and the text. You need to see what it's better in terms of being distinct enough to be perceived as an indicator of the overall status and subtle enough to don't be confused by a button or other component that requires an active action from the user via the UI.

Please create another icon for dark mode purposes that should be uploaded to the dark subfolder under images. The current icon doesn't look very readable (specially with the white inner path vs green outer shapes vs dark bakcground)

@pedropintosilva advise on the darkmode icon.

Also, I didn't see changes WRT how it looks when there is an error, please attach a screenshot if you can

pedropintosilva

^

pedropintosilva · 2025-01-20T09:39:56Z

I also see that there is only one commit, maybe it would be better to have one single commit (PR) that fixes the issue (linked in the description) so we can merge it as soon as it passes CI and then another PR with further UX improvements

banobepascal · 2025-01-21T06:45:59Z

I also see that there is only one commit, maybe it would be better to have one single commit (PR) that fixes the issue (linked in the description) so we can merge it as soon as it passes CI and then another PR with further UX improvements

Alignments of Icons, and Text in the server dialog

Will be creating another PR for UX improvements on the Status

pedropintosilva

It seems that now the dialog can become too narrow resulting in this ^

Also we could make sure that the text wraps if needed

banobepascal · 2025-01-21T20:58:46Z

Screencast.from.21-01-25.23.22.14.webm

banobepascal · 2025-01-22T05:14:09Z

@pedropintosilva please review

pedropintosilva

Additionally it seems there is a max-height being set elsewhere and the min-width of the #ServerAuditDialog-mainbox doesn't need to be so wide by default. Bellow is a the diff patch that I ended up having locally while reviewing your changes. Feel free to apply it with git apply pedroChangesOnTopOf10925pr.txt

pedroChangesOnTopOf10925pr.txt

pedropintosilva · 2025-01-22T11:01:50Z

browser/css/jsdialogs.css

+	grid-template-columns: auto 1fr 180px;
+}


I see 2 problems here:

I would prefer to avoid hard coded pixels and instead use ratios

Where I'm missing a pixel value is on the initial column (the icon)

grid-template-columns: var(--btn-img-size-s) 6fr 3fr;

Changed from pixel to ratios and used --btn-img-size-m as the ratio for the first column

browser/css/jsdialogs.css

pedropintosilva · 2025-01-22T11:02:51Z

browser/css/jsdialogs.css

+	grid-template-columns: auto 1fr 170px;
+}


I see 2 problems here:

I would prefer to avoid hard coded pixels and instead use ratios

Where I'm missing a pixel value is on the initial column (the icon)

grid-template-columns: var(--btn-img-size-s) 6fr 3fr;

Used 0fr to remove space for the unused icon instead of var(--btn-img-size-s)

browser/css/jsdialogs.css

pedropintosilva · 2025-01-22T11:07:26Z

browser/css/jsdialogs.css

+#ServerAuditDialog .ui-treeview-icon {
+	margin-inline-end: 2px;
+}


We don't need margins if we use column-gap as in:

.ui-treeview-headers, +.ui-treeview-entry { + column-gap: 8px; +}

removed margin-inline-end

pedropintosilva · 2025-01-22T11:07:37Z

browser/css/jsdialogs.css

+#ServerAuditDialog .ui-treeview-headers div.ui-treeview-icon-column {
+	margin-inline-end: 4px;


We don't need margins if we use column-gap as in:

.ui-treeview-headers, +.ui-treeview-entry { + column-gap: 8px; +}

Removed margin-inline-end

pedropintosilva · 2025-01-22T11:08:52Z

browser/css/jsdialogs.css

+#ServerAuditDialog .ui-treeview-entry > div:nth-last-child(1) {
+	margin-inline-start: 8px;
+}


We don't need margins if we use column-gap as in:

.ui-treeview-headers, +.ui-treeview-entry { + column-gap: 8px; +}

Using column-gap: 8px on ui-treeview-headers adds a gap between all three grid items, but we want to avoid adding extra space between the icon column and the status.

pedropintosilva · 2025-01-22T11:09:32Z

browser/css/jsdialogs.css

@@ -2214,6 +2238,7 @@ kbd,

 #ServerAuditDialog .ui-treeview {
 	border-color: transparent;
+	max-width: fit-content;


If previous comments I added we don't need this.

Removed max-width, but added max-height to max-content as the max-height was capped at 150px

.jsdialog:not(.sidebar).ui-treeview { max-height: 150px; }

@pedropintosilva please review

reduced the width of the server audit dailog as it doesn't need to be so wide by default. set a max-height to max content for server audit dialog Signed-off-by: Banobe Pascal <[email protected]> Change-Id: Ie7fa03d0009a55fb437417959c1f7d941b8c816f

pedropintosilva · 2025-01-23T13:12:24Z

browser/css/jsdialogs.css

+#ServerAuditDialog .ui-treeview-headers > div:nth-last-child(1) {
+	margin-inline-start: 7px;
+}


I would prefer to don't added this margin. I would prefer to add the inherited margin from the entries only when there is a single column, rather than applying the margin here to align with the entries. But let's merge this for now and we cna discuss this later.

banobepascal requested a review from pedropintosilva January 14, 2025 08:13

caolanm force-pushed the private/banobepascal/server-audit-dialog branch from 25ff897 to 4b37e37 Compare January 14, 2025 08:24

banobepascal force-pushed the private/banobepascal/server-audit-dialog branch 3 times, most recently from 2c65ee5 to 0294109 Compare January 19, 2025 22:42

pedropintosilva requested changes Jan 20, 2025

View reviewed changes

eszkadev added the draft label Jan 20, 2025

banobepascal force-pushed the private/banobepascal/server-audit-dialog branch 2 times, most recently from a736f0e to 9fe955d Compare January 20, 2025 16:24

pedropintosilva self-requested a review January 21, 2025 08:56

pedropintosilva requested changes Jan 21, 2025

View reviewed changes

pedropintosilva removed the draft label Jan 21, 2025

banobepascal force-pushed the private/banobepascal/server-audit-dialog branch from 9fe955d to 747b84c Compare January 21, 2025 20:37

banobepascal force-pushed the private/banobepascal/server-audit-dialog branch from 747b84c to e2c22b3 Compare January 22, 2025 05:13

pedropintosilva requested changes Jan 22, 2025

View reviewed changes

banobepascal force-pushed the private/banobepascal/server-audit-dialog branch from e2c22b3 to 6b32f3e Compare January 22, 2025 17:36

banobepascal force-pushed the private/banobepascal/server-audit-dialog branch from 6b32f3e to 8b7a893 Compare January 22, 2025 19:37

pedropintosilva self-requested a review January 23, 2025 13:06

pedropintosilva approved these changes Jan 23, 2025

View reviewed changes

pedropintosilva merged commit 7a50f22 into master Jan 24, 2025
14 checks passed

pedropintosilva deleted the private/banobepascal/server-audit-dialog branch January 24, 2025 08:39

banobepascal mentioned this pull request Jan 28, 2025

UX iprovements on server audit dialog #11036

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct text and icon alignments in server audit dialog #10925

Correct text and icon alignments in server audit dialog #10925

banobepascal commented Jan 14, 2025

banobepascal commented Jan 14, 2025

banobepascal commented Jan 20, 2025

pedropintosilva commented Jan 20, 2025

pedropintosilva left a comment

pedropintosilva commented Jan 20, 2025

banobepascal commented Jan 21, 2025

pedropintosilva left a comment

banobepascal commented Jan 21, 2025

banobepascal commented Jan 22, 2025

pedropintosilva left a comment

pedropintosilva Jan 22, 2025

banobepascal Jan 22, 2025

pedropintosilva Jan 22, 2025

banobepascal Jan 22, 2025 •

edited

Loading

pedropintosilva Jan 22, 2025

banobepascal Jan 22, 2025

pedropintosilva Jan 22, 2025

banobepascal Jan 22, 2025

pedropintosilva Jan 22, 2025

banobepascal Jan 23, 2025

pedropintosilva Jan 22, 2025

banobepascal Jan 22, 2025

banobepascal Jan 23, 2025

pedropintosilva Jan 23, 2025

		#ServerAuditDialog .ui-treeview-headers div.ui-treeview-icon-column {
		margin-inline-end: 4px;

Correct text and icon alignments in server audit dialog #10925

Correct text and icon alignments in server audit dialog #10925

Conversation

banobepascal commented Jan 14, 2025

Summary

IN (COOLWSD version: 24.04.8.1(git hash: 8475197 (E))

AFTER: #10870

CURRENT FIX AND IMPROVEMENT

Checklist

banobepascal commented Jan 14, 2025

banobepascal commented Jan 20, 2025

pedropintosilva commented Jan 20, 2025

pedropintosilva left a comment

Choose a reason for hiding this comment

pedropintosilva commented Jan 20, 2025

banobepascal commented Jan 21, 2025

pedropintosilva left a comment

Choose a reason for hiding this comment

banobepascal commented Jan 21, 2025

banobepascal commented Jan 22, 2025

pedropintosilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banobepascal Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IN (COOLWSD version: 24.04.8.1(git hash: `8475197` (E))

banobepascal Jan 22, 2025 •

edited

Loading