feat: add capability in FluentPersona for the name to appear before or after initials. #1750
Conversation
|
Hi, Thanks for contributing. One thing though...for this functionality I think it is better to use 'Start' and 'End' instead of 'Left' and 'Right' because we also support RTL layouts with these components. |
| @if (OnDismissClick.HasDelegate) | ||
| { | ||
| <div class="close"> | ||
| <FluentIcon Value="@(new CoreIcons.Regular.Size24.Dismiss())" Width="12px" Title="@DismissTitle" OnClick="OnDismissClick" /> | ||
| <FluentIcon Value="@(new CoreIcons.Regular.Size24.Dismiss())" Width="12px" Title="@DismissTitle" OnClick="OnDismissClick"/> |
There was a problem hiding this comment.
Please, don't remove this space.
| /// Gets or sets the position of the image. | ||
| /// </summary> | ||
| [Parameter] | ||
| public PersonaImagePosition? ImagePosition { get; set; } |
There was a problem hiding this comment.
I think it would be more logical to follow the "same philosophy" used by React:
by calling the new attribute TextPosition.
https://react.fluentui.dev/?path=/docs/components-persona--default#text-alignment
| /// <summary> | ||
| /// The tooltip is positioned to the left of the anchor element. | ||
| /// </summary> | ||
| Left, |
There was a problem hiding this comment.
As in my previous comment, and to follow Vincent's logic, it's probably better to use the names Before and After.
There was a problem hiding this comment.
We use start/end throughout the library (icon slots, alignment, ...) so I would stick with that. Don't recall we use before/after anywhere
| /// Gets or sets the position of the image. | ||
| /// </summary> | ||
| [Parameter] | ||
| public PersonaImagePosition? ImagePosition { get; set; } |
There was a problem hiding this comment.
I wouldn't put a nullable property, but with a default value of "Right/After".
| </div> | ||
| </FluentPresenceBadge> | ||
| </div> | ||
| @if (!ImagePosition.HasValue || ImagePosition == PersonaImagePosition.Left) |
There was a problem hiding this comment.
To avoid duplicating code in this file (depending on the value of ImagePosition), you could use CSS like this, based on a new position attribute to be defined on the div "main".
<div id="@Id" class="@ClassValue" position="@ImagePosition.ToAttributeValue()" ....fluent-persona[position='before'] .name {
order: 1;
margin-inline-end: 16px;
}
.fluent-persona[position='before'] .close {
order: 2;
}
.fluent-persona[position='before'] .initials {
order: 3;
}What do you think? Possible (to be verified if no side effects are detected).
There was a problem hiding this comment.
Done. It is cleaner. We might have to revisit this if the goal is feature-parity with fluentui react.
…r after initials. Code review feedback completed.
|
@microsoft-github-policy-service agree |
| @@ -62,4 +62,11 @@ | |||
|
|
|||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
There was a problem hiding this comment.
Can you remove these new lines. Not required in this csproj.
Pull Request
📖 Description
feat: add capability in FluentPersona for the name to appear before or after initials.
🎫 Issues
#1735
👩💻 Reviewer Notes
Unit test was updated to test the change. Demo application FluentPersona section was also updated with example.
📑 Test Plan
A new unit test was added.
✅ Checklist
General