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 modals in react #11714

Merged
merged 8 commits into from May 17, 2017

Conversation

Projects
None yet
5 participants
@stacey-gammon
Copy link
Contributor

commented May 10, 2017

Need to do it in a timeout because ng-react hasn’t loaded the react
code yet, so it can’t find the buttons.

Fixes #11681

Added tests which would have caught the issue, but also ended up switching the default button for deleting dashboards since it should be the non-destructive option by default.

@ycombinator

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

Functionality LGTM. Reviewing code...

@ycombinator
Copy link
Contributor

left a comment

The use of $timeout is clever but seems a bit hacky. Is this a well-understood pattern when using ng-react?

@stacey-gammon stacey-gammon force-pushed the stacey-gammon:fix-modals-in-react branch from c6ef171 to 59b60df May 15, 2017

@stacey-gammon

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

I agree it's hacky @ycombinator, unfortunately I couldn't come up with any other solution. I also couldn't find any approved pattern for this type of thing, with ng-react. The timeout just happened to work.

I added a comment, mentioning the limitations of the solution. If you have any other ideas, I'm open to exploring them. @weltenwort -- since you have some more experience with ng-react, have you ever come across this type of situation before, and maybe have a better solution?

@weltenwort

This comment has been minimized.

Copy link
Contributor

commented May 15, 2017

Focus management in React is not an easy topic, because setting the focus is a document-global effect and as such not covered by the React API.

I guess this cannot be achieved using the html autofocus attribute due to browser compatibility? In that case I would recommend to handle it in the componentDidMount lifecycle hook of the React component, in which the refs are already valid.

If you allow for an additional comment, the current use of data-test-subj in production code feels wrong to me. We should use some other class/attribute for that (in a previous project we used js- prefixed classes to make javascript dependencies easily searchable).

(For completely redux-based apps I would treat focus management as a side-effect in a middleware.)

@stacey-gammon stacey-gammon force-pushed the stacey-gammon:fix-modals-in-react branch from 59b60df to a7a61df May 15, 2017

@stacey-gammon

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

Oh much better idea @weltenwort - pushing the focusing logic into react - thanks!

cc @cjcenizal just FYI, I had to make KuiButton a class instead of a functional component for refs to work.

@stacey-gammon

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

Took it a step farther and also moved the esc key handling to the react component.

@stacey-gammon stacey-gammon force-pushed the stacey-gammon:fix-modals-in-react branch from 423e8dd to 65efd42 May 15, 2017

@ycombinator ycombinator self-requested a review May 16, 2017

@ycombinator

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

just FYI, I had to make KuiButton a class instead of a functional component for refs to work.

@stacey-gammon I was reading https://facebook.github.io/react/docs/refs-and-the-dom.html to understand refs and came across this statement:

You can, however, use the ref attribute inside a functional component as long as you refer to a DOM element or a class component.

Since you are adding a ref to the button element, I think you can leave the KuiButton component as a functional component?

@stacey-gammon

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

I think what that means is that I can use ref inside of a functional component e.g. this:

<button
  ref={ (kuiButton) => { this.kuiButton = kuiButton; } }
/>

but I still can't do this:

    <KuiButton
           type="hollow"
             data-test-subj="confirmModalCancelButton"
             ref={ (button) => { this.cancelButton = button; } }
             onClick={ onCancel }
           >

Without making KuiButton a class. In the second example, button comes back null when KuiButton is a functional component.

@ycombinator

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

Argh, I totally missed the use of ref on the KuiButton component. I only saw the use inside it on button. Oops!

@ycombinator
Copy link
Contributor

left a comment

Functionally (still) LGTM. Left a few minor comments on the code.

</KuiModalFooter>
</KuiModal>
);
export const CONFIRM_MODAL_BUTTONS = [

This comment has been minimized.

Copy link
@ycombinator

ycombinator May 16, 2017

Contributor

From what I can tell this const is only used within this module, as part of setting KuiConfirmModal.propTypes. Does it need to be exported?

super(props);

this.confirmButton;
this.cancelButton;

This comment has been minimized.

Copy link
@ycombinator

ycombinator May 16, 2017

Contributor

I think your intent behind these two lines was to clarify that these are properties of this object. For consistency, I think we should either do the same thing for this.kuiButton in the KuiButton component class or remove these two lines from here.

Also, if we choose to leave these here, I'd suggest initializing them explicitly to something like null. Otherwise their intent is not as clear, IMO.


onKeyDown = (event) => {
// Treat the 'esc' key as a cancel indicator.
if (event.keyCode === 27) {

This comment has been minimized.

Copy link
@ycombinator

ycombinator May 16, 2017

Contributor

I know this isn't part of this PR, but how do you feel about making the 27 here a constant somewhere? I see it used multiple times in the code so it might be nice for it to be defined once and then referred to as ESCAPE_KEY_CODE or something like that.

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal May 16, 2017

Contributor

I suggest creating a keyboard module in services.

This comment has been minimized.

Copy link
@stacey-gammon

stacey-gammon May 16, 2017

Author Contributor

I added a key_codes.js file in services that exports the consts individually. You think it should be something like this instead:

keyboard.js:

export const KeyCodes = {
  ESC_KEY: 27
};

I'm fine with either approach.

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal May 16, 2017

Contributor

key_codes.js works too!

stacey-gammon added some commits May 10, 2017

fix focused button not being set
Need to do it in a timeout because ng-react hasn’t loaded the react
code yet, so it can’t find the buttons.
add a test
that would have caught the issue, and also change the default focused
button for the listing delete page since it’s a destructive action.
Remove ESC tests in angular form
They don’t pass, I’m guessing because of the way it’s triggered, but
these test cases should now be handled in jest.  Also confirmed it
works as expected with the modal.

@stacey-gammon stacey-gammon force-pushed the stacey-gammon:fix-modals-in-react branch from 81bcf43 to 5522050 May 16, 2017

@stacey-gammon

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

So, I investigated autofocus a little more as an option because of the comments in this PR: #10925. It's supported in all the browsers we support, just not a couple mobile browsers, in which case, it's really not that important because the user needs to use the touch screen to press the right button anyway.

@cjcenizal

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

I've created an issue for making our Modals accessible: #11823. I don't think we need to implement this in this PR, but we should keep it in mind

@ycombinator
Copy link
Contributor

left a comment

LGTM!

@stacey-gammon stacey-gammon merged commit a9ee082 into elastic:master May 17, 2017

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request May 17, 2017

Fix modals in react (elastic#11714)
* fix focused button not being set

Need to do it in a timeout because ng-react hasn’t loaded the react
code yet, so it can’t find the buttons.

* add a test

that would have caught the issue, and also change the default focused
button for the listing delete page since it’s a destructive action.

* Add a comment

* Push focusing of default button into react code

* Push ESC key handling into react as well

* Remove ESC tests in angular form

They don’t pass, I’m guessing because of the way it’s triggered, but
these test cases should now be handled in jest.  Also confirmed it
works as expected with the modal.

* Address code review comments

* Use autoFocus

stacey-gammon added a commit that referenced this pull request May 17, 2017

Fix modals in react (#11714) (#11862)
* fix focused button not being set

Need to do it in a timeout because ng-react hasn’t loaded the react
code yet, so it can’t find the buttons.

* add a test

that would have caught the issue, and also change the default focused
button for the listing delete page since it’s a destructive action.

* Add a comment

* Push focusing of default button into react code

* Push ESC key handling into react as well

* Remove ESC tests in angular form

They don’t pass, I’m guessing because of the way it’s triggered, but
these test cases should now be handled in jest.  Also confirmed it
works as expected with the modal.

* Address code review comments

* Use autoFocus

snide added a commit to snide/kibana that referenced this pull request May 30, 2017

Fix modals in react (elastic#11714)
* fix focused button not being set

Need to do it in a timeout because ng-react hasn’t loaded the react
code yet, so it can’t find the buttons.

* add a test

that would have caught the issue, and also change the default focused
button for the listing delete page since it’s a destructive action.

* Add a comment

* Push focusing of default button into react code

* Push ESC key handling into react as well

* Remove ESC tests in angular form

They don’t pass, I’m guessing because of the way it’s triggered, but
these test cases should now be handled in jest.  Also confirmed it
works as expected with the modal.

* Address code review comments

* Use autoFocus

Dreadnoth added a commit to Dreadnoth/kibana that referenced this pull request Aug 8, 2017

Fix modals in react (elastic#11714) (elastic#11862)
* fix focused button not being set

Need to do it in a timeout because ng-react hasn’t loaded the react
code yet, so it can’t find the buttons.

* add a test

that would have caught the issue, and also change the default focused
button for the listing delete page since it’s a destructive action.

* Add a comment

* Push focusing of default button into react code

* Push ESC key handling into react as well

* Remove ESC tests in angular form

They don’t pass, I’m guessing because of the way it’s triggered, but
these test cases should now be handled in jest.  Also confirmed it
works as expected with the modal.

* Address code review comments

* Use autoFocus

@stacey-gammon stacey-gammon deleted the stacey-gammon:fix-modals-in-react branch Oct 24, 2017

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