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

fix(Modal): FocusTrap works with showClose being false #2774

Merged
merged 8 commits into from Aug 30, 2019

Conversation

@SpyTec
Copy link
Contributor

SpyTec commented Aug 26, 2019

What: Fixes FocusTrap breaking on Modal if showClose prop is false.

Additional issues: Fixes #2757

I have not been able to test this out as I cannot build PatternFly at the moment.

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Aug 26, 2019

PatternFly-React preview: https://patternfly-react-pr-2774.surge.sh

@SpyTec

This comment has been minimized.

Copy link
Contributor Author

SpyTec commented Aug 26, 2019

Just noticed that this breaks the Modal (no header). But removing actions prop from it does not crash it at least

<Backdrop>
<FocusTrap focusTrapOptions={{ clickOutsideDeactivates: true }} className={css(styles.bullseye)}>
<ModalBox
let modalBox = (

This comment has been minimized.

Copy link
@redallen

redallen Aug 27, 2019

Contributor

I like this change, but it's hard to read because you mutate modalBox. Can we make this a const?

</ModalBox>
);
// Only add FocusTrap if close button exists to prevent errors
if (showClose) {

This comment has been minimized.

Copy link
@redallen

redallen Aug 27, 2019

Contributor

You can remove this if statement and instead include it in the JSX as a ternary statement.

}
return (
<Backdrop>
{modalBox}

This comment has been minimized.

Copy link
@redallen

redallen Aug 27, 2019

Contributor

How about

{showClose
  ? (
    <FocusTrap focusTrapOptions={{ clickOutsideDeactivates: true }} className={css(styles.bullseye)}>
      {modalBox}
    </FocusTrap>
  )
  : (
    <div className={css(styles.bullseye)}>
        {modalBox}
    </div>
  )
}
@SpyTec

This comment has been minimized.

Copy link
Contributor Author

SpyTec commented Aug 28, 2019

@redallen I've updated based on your feedback, thanks :)

I also took the liberty of using Bullseye instead of css when FocusTrap isn't used.

Copy link
Contributor

redallen left a comment

LGTM!

Copy link
Member

cdcabrera left a comment

So quick question on this solution

The solution here looks similar to what's on the Readme for Focus Trap... https://github.com/davidtheclark/focus-trap-react#readme however there look like a couple of other props that I'm curious to know if we tried as well? active which looks promising and paused which might not be what I think it is...

@SpyTec

This comment has been minimized.

Copy link
Contributor Author

SpyTec commented Aug 28, 2019

I didn't even consider testing those props, I'll give it a try to see if we can just use that instead :)

I noticed in one of the other PF4 components that we went with the solution that's currently in this PR

Copy link
Member

jeff-phillips-18 left a comment

I don't think we want to disable the focus trap just because there is no close button. We want the focus to stay in the modal until closed for those modals that DO have focusable elements. I believe it would be an exception to have a modal w/ no focusable elements.

I would suggest if we want to support modals w/o any focusable elements we add a prop to specify not adding the focus trap.

@SpyTec

This comment has been minimized.

Copy link
Contributor Author

SpyTec commented Aug 28, 2019

Sounds like a good idea to me, though is there a way to hint this when an error is thrown if a user stumbles upon the issue? Such as catching the error and adding a message to it about the prop.

@jeff-phillips-18

This comment has been minimized.

Copy link
Member

jeff-phillips-18 commented Aug 28, 2019

You get extra credit for that 😉

@SpyTec

This comment has been minimized.

Copy link
Contributor Author

SpyTec commented Aug 29, 2019

Added disableFocusTrap prop. FocusTrap should now be active at all times unless the prop is true.

I also fixed ModalContent documentation to have description, which it didn't before

<FocusTrap focusTrapOptions={{ clickOutsideDeactivates: true }} className={css(styles.bullseye)}>
<ModalBox
const modalBox = (
<ModalBox
style={boxStyle}
className={className}
isLarge={isLarge}
isSmall={isSmall}
title={title}
id={ariaDescribedById || id}
>

This comment has been minimized.

Copy link
@jeff-phillips-18

jeff-phillips-18 Aug 29, 2019

Member

Nit: indentation

@jeff-phillips-18

This comment has been minimized.

Copy link
Member

jeff-phillips-18 commented Aug 29, 2019

@SpyTec Don't see the doc change to Modal though the commit message indicates you meant to include it.

Co-Authored-By: Jeff Phillips <jephilli@redhat.com>
@SpyTec

This comment has been minimized.

Copy link
Contributor Author

SpyTec commented Aug 30, 2019

Sorry, glanced over that part. Fixed Modal prop description :)

Copy link
Member

jeff-phillips-18 left a comment

LGTM! Thanks @SpyTec

@redallen redallen merged commit aa23ca8 into patternfly:master Aug 30, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Aug 30, 2019

Your changes have been released in:

  • @patternfly/react-core@3.94.3
  • @patternfly/react-docs@4.10.34
  • @patternfly/react-inline-edit-extension@2.11.9
  • demo-app-ts@2.21.13
  • @patternfly/react-table@2.19.9
  • @patternfly/react-topology@2.8.9
  • @patternfly/react-virtualized-extension@1.1.140

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.