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

[FluentPersona] Hide name functionality #1706

Closed
wants to merge 3 commits into from
Closed

[FluentPersona] Hide name functionality #1706

wants to merge 3 commits into from

Conversation

lokitech
Copy link

Pull Request

📖 Description

Changes involve the addition of a new feature that allows only the initials of a person to be rendered. This is achieved through the introduction of a new HideName property in the FluentPersona class. Additionally, the FluentPersona.razor file has been updated to accommodate this new feature, as well as not rendering <div class="name"> when both ChildContent and Name properties are null. Changes also include additional attributes to be passed to the div element in order for events to work (e.g. @onmouseover).

🎫 Issues

FluentPersona component renders <div class="name"> even if both Name and ChildContent properties are empty.

fluent-ui-persona-initials-noname-bug-html

Which results in the element taking additional empty space within the container.

fluent-ui-persona-initials-noname-bug

This can cause problems when using FluentPersona component as a button or when trying to align it with other elements.

Additionally FluentPersona component does not have the ability to use events like @onmouseover.

👩‍💻 Reviewer Notes

  1. A new demo section titled "Show only initials" has been added to the PersonaPage.razor file. This section describes a new feature where only the initials of a person are shown when the HideName property is set to true or when the Name and ChildContent properties are not set.

fluent-ui-persona-initials-noname-fix

fluent-ui-persona-initials-noname-fix-html

  1. The div tag in the FluentPersona.razor file has been updated to include @attributes="@AdditionalAttributes". This allows for additional attributes to be passed to the div element.

  2. The name div in the FluentPersona.razor file has been updated to only display if the HideName property is false and either the Name property is not null or whitespace, or the ChildContent property is not null.

  3. A new property HideName has been added to the FluentPersona class in the FluentPersona.razor.cs file. This property is a boolean that determines whether the name should be displayed or not.

📑 Test Plan

FluentPersonaTests has been updated to accommodate these new changes.

✅ Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new compontent
  • I have modified an existing component
  • I have validate Unit Tests for an existing component

…oes not render <div class="name> when true.

- Added functionality of only displaying <div class="name> if the `HideName` property is false and either the `Name` property is not null or whitespace, or the `ChildContent` property is not null.
- Added `@attributes="@AdditionalAttributes"` to `FluentPersona.razor` <div> tag
- Added example to `PersonPage.razor`
- Added unit tests
…oes not render <div class="name> when true.

- Added functionality of only displaying <div class="name> if the `HideName` property is false and either the `Name` property is not null or whitespace, or the `ChildContent` property is not null.
- Added `@attributes="@AdditionalAttributes"` to `FluentPersona.razor` <div> tag
- Added example to `PersonPage.razor`
- Added unit tests
@lokitech
Copy link
Author

@microsoft-github-policy-service agree

@dvoituron
Copy link
Collaborator

Thank you for your excellent work. Two comments:

  1. I have updated the FluentPersona component to not display the Name in this active PR: [FluentProfileMenu] New component #1705
  2. Why did you add a new HideName property and not use the nullable Name property?

@lokitech
Copy link
Author

No problem, happy to contribute. To answer your comments:

  1. I saw your pull request, but did not check your changes which I should have. I stupidly assumed your new component does not use FluentPersona. I'm glad to see you've also included null/empty check to Name and ChildComponent, as well as added the @attributes tag.
  2. I've added a separate property HideName for instances where a user might want to hide or show name in certain scenarios (e.g. name is shown only when the mouse hovers over the component). I've utilized this in PersonaPage changes, code below.
<FluentPersona Name="Lydia Bauer"
               ImageSize="50px"
               HideName=@hideName
               @onmouseover="OnMouseOver"
               @onmouseout="OnMouseOut"
               Status="PresenceStatus.Busy"
               StatusSize="PresenceBadgeSize.Small">
</FluentPersona>

@code {
    bool hideName = true;

    private void OnMouseOver()
    {
        hideName = false;
    }

    private void OnMouseOut()
    {
        hideName = true;
    }
}

@dvoituron
Copy link
Collaborator

@lokitech

I've added a separate property HideName for instances where a user might want to hide or show name in certain scenarios
(e.g. name is shown only when the mouse hovers over the component). I've utilized this in PersonaPage changes, code below.

Yes, I understand the purpose of this property, but it seems unnecessary when it is possible to allow the Name property to be emptied. That's what I did in my previous PR.

I've just extracted only the FluentPersona update code to make it clearer: #1710.

In combination with @onmouseover and @onmouseout, you can get the same result by modifying the contents of the Name attribute.

image

<FluentPersona Name="Lydia Bauer"
               ImageSize="50px"
               Status="PresenceStatus.Busy"
               StatusSize="PresenceBadgeSize.Small">
</FluentPersona>

<FluentPersona Initials="LB" ImageSize="50px" />

<FluentPersona ImageSize="50px" />

@lokitech
Copy link
Author

Yeah I understand you can do it by modifying the Name property.
I've just added the HideName as an extra feature so Name doesn't have to be nulled/emptied and then set again. It's by no means a necessary thing, just something I thought would be nice to have, but if you think it would just clutter the class, I also understand.

@dvoituron
Copy link
Collaborator

Yeah I understand you can do it by modifying the Name property. I've just added the HideName as an extra feature so Name doesn't have to be nulled/emptied and then set again. It's by no means a necessary thing, just something I thought would be nice to have, but if you think it would just clutter the class, I also understand.

I don't think that's a good thing. Yes, it's probably simpler in your case, but then we could add HideInitials, HideImage, etc :-)
In fact, you should keep the name in another variable to obtain the same result. Or adapt the .name style to your needs.

@vnbaaij
Copy link
Collaborator

vnbaaij commented Mar 18, 2024

So, I'm assuming this PR canbe closed then as the gist of it is now part of #1710?

@lokitech
Copy link
Author

Yeah I understand you can do it by modifying the Name property. I've just added the HideName as an extra feature so Name doesn't have to be nulled/emptied and then set again. It's by no means a necessary thing, just something I thought would be nice to have, but if you think it would just clutter the class, I also understand.

I don't think that's a good thing. Yes, it's probably simpler in your case, but then we could add HideInitials, HideImage, etc :-) In fact, you should keep the name in another variable to obtain the same result. Or adapt the .name style to your needs.

You're totally right! Closing it now.
Have a great day!

@lokitech lokitech closed this Mar 18, 2024
@dvoituron
Copy link
Collaborator

You're totally right! Closing it now. Have a great day!

Thank you, and don't hesitate to suggest new features / PR. You've done a great job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants