Skip to content

Conversation

ryami333
Copy link
Owner

@ryami333 ryami333 commented Mar 6, 2018

Basically an upstream-retrofit of the logic from acc-css-app to add touch-away handling for iOS devices who don't actually trigger a 'blur' on focussed elements when touching-away from the element.

@ryami333 ryami333 requested a review from janzenz March 6, 2018 22:09
closeButton = wrapper.find(CloseButton);
});

it(`${React.version} - asdmatches the previous snapshot`, () => {
Copy link
Owner Author

Choose a reason for hiding this comment

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

This stuff is pretty much all just indentation changes, as I wrapped them in an extra describe declaration.

);
});

describe('touch devices -', () => {
Copy link
Owner Author

Choose a reason for hiding this comment

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

New tests start here

handleTouch = ({ target }: Event) => {
const { activeElement } = document;

if (!(activeElement instanceof Element) || !(target instanceof Element))
Copy link
Owner Author

Choose a reason for hiding this comment

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

In acc-css-app we're testing for instanceof HTMLElement, but this would not catch HTMLBodyElement for example. Element is a bit of a broader net.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aren't we going to apply this as well at line 70?

Copy link
Owner Author

Choose a reason for hiding this comment

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

We could, but it's a bit of a moot point because it's not possible for a label component to be an HTMLBodyElement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup. Fair enough.

};

hide = (): void => {
hide = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is keeping void a problem?

Copy link
Owner Author

Choose a reason for hiding this comment

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

For some reason it breaks syntax highlighting in my IDE :(

It doesn't matter though because Flow will infer the return types.

Copy link
Collaborator

@janzenz janzenz left a comment

Choose a reason for hiding this comment

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

No problem found, just a few questions.

Copy link
Collaborator

@janzenz janzenz left a comment

Choose a reason for hiding this comment

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

All good!

@ryami333 ryami333 merged commit f935f4d into master Mar 6, 2018
@ryami333 ryami333 deleted the hotfix/ipad-fix branch March 6, 2018 22:31
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.

2 participants