Skip to content

Add visible labels to resource select component and filter dialog#3727

Merged
adamint merged 7 commits intomicrosoft:mainfrom
adamint:dev/adamint/3396-add-visible-labels
Jul 31, 2024
Merged

Add visible labels to resource select component and filter dialog#3727
adamint merged 7 commits intomicrosoft:mainfrom
adamint:dev/adamint/3396-add-visible-labels

Conversation

@adamint
Copy link
Copy Markdown
Member

@adamint adamint commented Apr 16, 2024

Fixes #3396, we need to have visible labels for interactive components. We also had several kinds of All/None options, so I standardized these into (All) and (None)

I added labels to the inputs in FilterDialog and made them the same width.
image

The resource select also has a label to its left.
image

I removed the label to the right of the resource select on the console log page when there is no resource selected, as it just mentioned that there was no resource selected, which is redundant with the select's (None) selected text.

image

Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the area-dashboard label Apr 16, 2024
@adamint
Copy link
Copy Markdown
Member Author

adamint commented Apr 26, 2024

@tlmii @JamesNK

@JamesNK
Copy link
Copy Markdown
Member

JamesNK commented Apr 27, 2024

"Select a resource" is slightly higher than center. For some reason CSS adds a small margin:
image

Override the CSS when inside the toolbar to have no margin top or bottom.

image

Structured logs page has "Level:" with a colon. Should stay consistent with that. Add colon after "Select a resource".

Also, the "Level:" here isn't a label. It should probably be.

@adamint
Copy link
Copy Markdown
Member Author

adamint commented May 1, 2024

@JamesNK I just want to confirm that !important is necessary since fluentui-blazor uses a class and attribute selector (.fluent-input-label[attribute])? If not, do you know how to avoid !important here?

@tlmii
Copy link
Copy Markdown
Member

tlmii commented May 1, 2024

@JamesNK I just want to confirm that !important is necessary since fluentui-blazor uses a class and attribute selector (.fluent-input-label[attribute])? If not, do you know how to avoid !important here?

Making the selector .layout fluent-toolbar > .fluent-input-label would give it a specificity of 21, beating the built-in rule's specificity of 20. Seems like a superfluous addition, but it does make a difference and avoids the !important which always feels like a code smell.

That misalignment feels like its a bug in the library, though, since we're just using the Label attribute of the FluentSelect component and its generating the rest. @vnbaaij thoughts on that?

@vnbaaij
Copy link
Copy Markdown
Contributor

vnbaaij commented May 1, 2024

Structured logs page has "Level:" with a colon. Should stay consistent with that. Add colon after "Select a resource".

Also, the "Level:" here isn't a label. It should probably be.

In the Fluent 2 guides (https://fluent2.microsoft.design/components/web/react/field/usage#layout) no colons are used for labels. I would rather remove them instead of adding them.

@adamint
Copy link
Copy Markdown
Member Author

adamint commented May 1, 2024

Removed colons @vnbaaij, @tlmii switched selectors

@vnbaaij
Copy link
Copy Markdown
Contributor

vnbaaij commented May 1, 2024

That misalignment feels like its a bug in the library, though, since we're just using the Label attribute of the FluentSelect component and its generating the rest. @vnbaaij thoughts on that?

Going with the Fluent 2 desing, we assume the label is placed above the component it belongs to. We need that margin-bottom to prevent things from getting too cramped:
image

What I started doing based on this issue, was adding a Orientation parameter to the FluentInputLabel (which is the component being rendered when you set the Label on the FluentSelect) that will add a 'fluent-input-label-horizontal' class (setting margin-bottom: 0;) in the case of setting it to Orientation.Horizontal.
Now, while that is a nice addition, it has no use in this case as you are setting the Label property on the Select and there is no way to specify the orientation.
When this addition is included in the library, you could remove the Label parameter from the Select and add a FluentInputLabel with a Orientation="Orientation.Horizontal" manually:

image

@vnbaaij
Copy link
Copy Markdown
Contributor

vnbaaij commented May 1, 2024

PR: microsoft/fluentui-blazor#1994

Adam Ratzman added 2 commits July 18, 2024 13:08
@adamint adamint force-pushed the dev/adamint/3396-add-visible-labels branch from 4dd6032 to dcac549 Compare July 18, 2024 17:34
@adamint
Copy link
Copy Markdown
Member Author

adamint commented Jul 18, 2024

@vnbaaij @JamesNK resurrecting this after the last pr got merged. Same changes (after prior feedback applied), and uses the same (All) and (None) strings everywhere.

image
image
image
image
image

@vnbaaij
Copy link
Copy Markdown
Contributor

vnbaaij commented Jul 18, 2024

@adamint sorry, but I don't understand what exactly is the issue now (going on these screenshots). Can you clarify?

@adamint
Copy link
Copy Markdown
Member Author

adamint commented Jul 18, 2024

@vnbaaij #3396 is still active, this PR adds labels to the filter dialog input components, as well as the resource select

@adamint
Copy link
Copy Markdown
Member Author

adamint commented Jul 22, 2024

@JamesNK as well

@JamesNK
Copy link
Copy Markdown
Member

JamesNK commented Jul 25, 2024

Change "Select a resource" to "Resource" on all the pages:
image
This is consistent with other UI in the toolbar, e.g. there is just the text "Filter", not "Select a filter".

Remove "Value" placeholder:
image
The other fields here are pre-selected. It could be confusing that value is also pre-selected.

I think after these changes this is ready to merge.

@adamint
Copy link
Copy Markdown
Member Author

adamint commented Jul 30, 2024

Change "Select a resource" to "Resource" on all the pages: image This is consistent with other UI in the toolbar, e.g. there is just the text "Filter", not "Select a filter".

Remove "Value" placeholder: image The other fields here are pre-selected. It could be confusing that value is also pre-selected.

I think after these changes this is ready to merge.

Changed both @JamesNK

Comment thread src/Aspire.Dashboard/Resources/ControlsStrings.resx Outdated
adamint added 3 commits July 31, 2024 10:33
# Conflicts:
#	src/Aspire.Dashboard/Resources/ControlsStrings.Designer.cs
#	src/Aspire.Dashboard/wwwroot/css/app.css
@adamint adamint enabled auto-merge (squash) July 31, 2024 14:37
@adamint adamint merged commit 4adf0e7 into microsoft:main Jul 31, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

4 participants