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

[EuiConfirmModal] Added ownFocus and isLoading props. #4421

Merged
merged 6 commits into from
Feb 2, 2021

Conversation

ashikmeerankutty
Copy link
Contributor

@ashikmeerankutty ashikmeerankutty commented Jan 12, 2021

Summary

Fixes #4411

Added ownFocus and isLoading props to EuiConfirmModal

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs
  • Added documentation
    - [ ] Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4421/

@thompsongl
Copy link
Contributor

Thanks, @ashikmeerankutty!

@cchaos, a couple things you mention in the issue:

I've also been wanting to add the overlay mask as a direct usage mainly as a way to make sure it is rendered since all modals really should have it

Consider making ownFocus on by default?

I'll cross that one off the list, but also leave it there to remind ourselves to add a sentence about it in the guidelines.

Something you want to tack on to this PR?

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Consider making ownFocus on by default?

We talked about this as a team and overall we think that it was mostly a mistake not to have included the overlay mask directly within EuiModal by default and always. EuiModal is not similar to EuiFlyout where a user could still interact with the page content below. Therefore, we will always want to obscure the content beneath the modal with the mask and we already restrict focus to within the modal too.

It does mean that most modal usages will now double display an overlay mask, so we'll want to ensure communication and/or make this change in downstream apps.

Therefore, @ashikmeerankutty , Can you remove optional ownFocus prop and always render the mask? Then we'll want to remove all manual instances of <EuiOverlayMask> within the examples.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4421/

@ashikmeerankutty
Copy link
Contributor Author

@cchaos Sorry for the delay updated the PR. Please do review.

@cchaos
Copy link
Contributor

cchaos commented Feb 1, 2021

Jenkins, test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4421/

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM. Just a few language polish comments. Also, you'll need to update with master so that you get the latest Changelog and can move your item to the right spot.

src/components/modal/confirm_modal.tsx Outdated Show resolved Hide resolved
src/components/modal/confirm_modal.test.tsx Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@ashikmeerankutty
Copy link
Contributor Author

@cchaos Thanks for the review. Updated the PR with the changes.

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks, @ashikmeerankutty!

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4421/

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.

[EuiConfirmModal] Add loading state to confirm button and ownFocus for overlay mask
4 participants