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

Proposed tweaks for FocusTrap #419

Merged
merged 3 commits into from
Aug 29, 2022
Merged

Conversation

DannyRichardsonWG
Copy link
Contributor

Hi Chris,

I've got custom dialogs in my setup and was having trouble with the preview packages. These tweaks sorted it for me.

Null check on FocusTrap in OnAfterRenderAsync

  • If using custom dialogs _focusTrap is not set but there's no null check on the render

Make FocusTrap field public so it can be used in custom dialogs

  • Changing to public means we can add FocusTrap in custom dialogs if we want to (and the null ref check prevents errors if we don't)
  • Renamed _focusTrap to FocusTrap and set the variable annotation as nullable

I was tempted to change

    private void AttemptFocus() 
        => _setFocus = FocusTrap != null;

But wasn't sure if there's a time before the first render when the @ref gets set that AttemptFocus might be called

Cheers,

Danny

Copy link
Member

@chrissainty chrissainty left a comment

Choose a reason for hiding this comment

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

This all looks good to me bar the small change around the FocusTrap field. 👍

@@ -28,7 +28,7 @@ public partial class BlazoredModalInstance : IDisposable

[SuppressMessage("Style", "IDE0044:Add readonly modifier", Justification = "This is assigned in Razor code and isn't currently picked up by the tooling.")]
private ElementReference _modalReference;
private FocusTrap _focusTrap = default!;
public FocusTrap? FocusTrap = default;
Copy link
Member

Choose a reason for hiding this comment

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

If we're making this public I think it would be better to swap to a property. As it's now nullable, we can remove the default assignment as well. Could you also move this a few lines up so it's with the other properties?

Suggested change
public FocusTrap? FocusTrap = default;
public FocusTrap? FocusTrap { get; set; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Chris,

Sure, changes are in now.

@chrissainty chrissainty added the Bug Something isn't working label Aug 28, 2022
@chrissainty chrissainty merged commit 9833bd3 into Blazored:main Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants