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

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

Merged
merged 7 commits into from
Jul 31, 2024

Conversation

adamint
Copy link
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

@adamint
Copy link
Member Author

adamint commented Apr 26, 2024

@tlmii @JamesNK

@JamesNK
Copy link
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
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
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
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
Member Author

adamint commented May 1, 2024

Removed colons @vnbaaij, @tlmii switched selectors

@vnbaaij
Copy link
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
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
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
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
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
Member Author

adamint commented Jul 22, 2024

@JamesNK as well

@JamesNK
Copy link
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
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

# 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 dotnet:main Jul 31, 2024
11 checks passed
@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