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

[InfoButton] New component #2332

Open
wants to merge 29 commits into
base: dev
Choose a base branch
from
Open

Conversation

franklupo
Copy link
Contributor

src/Core/Components/InfoButton/FluentInfoButton.razor Outdated Show resolved Hide resolved
src/Core/Components/InfoButton/FluentInfoButton.razor Outdated Show resolved Hide resolved
}

[Fact]
public void FluentInfoButton_Default()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add other Unit Test to validate all properties (one test by property)?

src/Core/Components/InfoButton/FluentInfoButton.razor.cs Outdated Show resolved Hide resolved
src/Core/Components/InfoButton/FluentInfoButton.razor.cs Outdated Show resolved Hide resolved

namespace Microsoft.FluentUI.AspNetCore.Components;

public partial class FluentInfoButton
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component must be accessible (you can use NVDA to validate it).
Example (playing with sound):

  • Enter to open and read the tooltip content
  • Tab to navigate in the tooltip
  • Escape to close the tooltip
InfoButton.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to change from tooltip to popover, because it seems to me that the input management is better. I don't know how to display corner. How can I do it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't. At least not with the in-built capabilities. You would have to resort to CSS and JS to make it work which seems to me a bit overkill for something that is already there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I manage the focus on the tooltip?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what to do. I ask for help.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No :-(

  • Not accessible (Enter, ESC, Read and navigate tooltip content)
  • Unit tests fail

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for my distraction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how to test? aria-describedby is created every time.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IdTooltip must be a concatenation of the component ID and a fixed string like $"{Id}-tooltip". So in your tests, you can set an Id and your HTML results will be invariant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the test and removed AdditionalAttributes because it gives an error:

The property 'AdditionalAttributes' on component type 'Microsoft.FluentUI.AspNetCore.Components.FluentTooltip' cannot be set explicitly when also used to capture unmatched values. Unmatched values: role

@PascalVorwerk
Copy link

Hi! Is the InfoButton still planned for a upcoming release? Got a use case for it, so just checking before i implement my own version :P

@vnbaaij
Copy link
Collaborator

vnbaaij commented Oct 21, 2024

Hi! Is the InfoButton still planned for a upcoming release? Got a use case for it, so just checking before i implement my own version :P

@franklupo is quite busy it seems.

What if you just take the code from this PR, address the remarks Denis and I posted and submit a new PR? We'll then review that one and include it in an upcoming version (all dependent on time we all need) if all is okay. Probably still less work than creating a new one yourself and as a bonus you'd be helping us and the community. Sounds good?

@PascalVorwerk
Copy link

Hi! Is the InfoButton still planned for a upcoming release? Got a use case for it, so just checking before i implement my own version :P

@franklupo is quite busy it seems.

What if you just take the code from this PR, address the remarks Denis and I posted and submit a new PR? We'll then review that one and include it in an upcoming version (all dependent on time we all need) if all is okay. Probably still less work than creating a new one yourself and as a bonus you'd be helping us and the community. Sounds good?

Hey! I have to check whether I can invest time in that, depending on how much its needed right now. But I will keep it in mind!

@franklupo
Copy link
Contributor Author

Hi guys,
Sorry, but this is not a good time for me.
Can you handle it @vnbaaij ?
Thanks

@PascalVorwerk
Copy link

Hey, I just did some work on the unit tests that were still needed. Was there any other work still open in this PR? It's a pretty long log so I'm not sure.

I tried to open up a PR but I am getting a unauthorized response, do I need to get some rights in order to open it? Storing the changes locally now :)

@vnbaaij
Copy link
Collaborator

vnbaaij commented Oct 28, 2024

You need to create a PR in your fork of our repo. It should work then.

Once you're done, we need to go through all earlier unresolved comments and check those.

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.

4 participants