fluentui-blazor icon indicating copy to clipboard operation
fluentui-blazor copied to clipboard

[Card] Fluent UI 2 Card and FocusManager

Open andreisaperski opened this issue 1 year ago • 7 comments

Pull Request

📖 Description

  • Optional Fluent UI 2 Card look and behavior, it's activated when FluentCardHeader, FluentCardPreview or FluentCardFooter is added to card's content. Implementation is a Blazor replica of Card component from Fluent UI React. Card's CSS uses v2 design tokens by default and v1 tokens as fallback.

  • FocusManager - helps to control focus behavior, it's basically a wrapper for tabster which is used in Fluent UI React and Fluent UI Web Components v3. Added because of Card's FocusMode.

  • Rendering of tabindex attribute was added to FluentButton - libraries, including tabster, usually use 'standard' query selector to find focusable elements. This selector includes standard elements like a, input, button etc., and elements with tabindex attribute, as a result Fluent UI Web Components can be programmatically identified as focusable only if they have tabindex attribute.

✅ Checklist

General

  • [x] I have added tests for my changes.
  • [x] I have tested my changes.
  • [x] I have updated the project documentation to reflect my changes.
  • [x] 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
  • [x] I have modified an existing component
  • [x] I have validate Unit Tests for an existing component

andreisaperski avatar Dec 30 '23 17:12 andreisaperski

I found this PR very very large (141 updated files), probably because there are several features included in this one PR 🤪

Give us a few days to check and verify all your code.

dvoituron avatar Dec 30 '23 18:12 dvoituron

Probably, half of these files are *.verified.html files - most of them are files affected by 'tabindex' added for fluent-button, other are new tests for Card)

andreisaperski avatar Dec 30 '23 18:12 andreisaperski

Yes. I saw that.

Is it possible to avoid inserting a default tabindex in existing components? But only when the developer wants to include this new functionality?

I would prefer not to modify the generated code of all the components, which could be considered as a breaking change.

dvoituron avatar Dec 30 '23 19:12 dvoituron

Yeah, having it rendered all the time was bugging me as well... From the top of my head, the following could be done to mitigate this issue:

  1. add EmitTabIndexAttribute property to LibraryConfiguration
  2. add internal static bool EmitTabIndexAttribute = false; to FluentComponentBase
  3. add FluentComponentBase.EmitTabIndexAttribute = options.EmitTabIndexAttribute; to ServiceCollectionExtensions.AddFluentUIComponents
  4. modify FluentComponentBase.GetTabIndexAttributeValue to return null if EmitTabIndexAttribute is set to null - tabindex won't be rendered in this case This is `opt-in' for the whole app. What do you think about this approach or maybe you have something else on your mind?

andreisaperski avatar Dec 30 '23 19:12 andreisaperski

That sounds like a solid approach.

I'm a bit worried about using/including the tabster package as the repo mentions it is all a bit experimental.

Another thing is I do not 'just' want to blindly copy Fluent React components. We tried that before with the BlazorFluentUI library and we just could not keep up. I need to check with the fluent team to see what the plans are for the Card web component. Not shooting this down but let's take some time to do it right (so it will not be part of our next release (4.3))

vnbaaij avatar Dec 30 '23 20:12 vnbaaij

Re tabster, yes, it says it's still WIP PoC, but Fluent React is using it for [Focus Management] (https://react.fluentui.dev/?path=/docs/utilities-focus-management-usearrownavigationgroup--default) and it gave a courage to try it 🙃

Re WC v3, i was monitoring the activity in v3 branch for the past six month - besides the spike of commits related to unit tests, there were no significant changes. Basically, that's why i decided to port Card.

andreisaperski avatar Dec 30 '23 20:12 andreisaperski

Re WC v3, i was monitoring the activity in v3 branch for the past six month - besides the spike of commits related to unit tests, there were no significant changes. Basically, that's why i decided to port Card.

Understood. Nevertheless I will check with the team. We are trying to get all Fluent activities closer aligned and I believe in the end that will be better for everyone.

vnbaaij avatar Dec 30 '23 21:12 vnbaaij

Was looking at this again and I now see for the WC v3 they are using "tabbable": "^6.2.0". Think it makes sense for us to use that then as well

vnbaaij avatar Apr 24 '24 10:04 vnbaaij

I'm closing this one for 2 reasons.

  1. This uses a different helper js script (tabster/tabbable)
  2. We intruduce Fluent2 design for 1 specific component

I would still be interested in getting the FocusManager/tabindex work in, but think it is better to have a separate PR for that (and using the new js helper)

vnbaaij avatar Apr 26 '24 16:04 vnbaaij