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: Remove focus trap on AnnotationPopover on mobile #279

Merged
merged 9 commits into from
Nov 4, 2018
Merged
3 changes: 2 additions & 1 deletion src/Annotator.scss
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,8 @@

// Quad point positioning - the helper divs are positioned relative to the Rangy-created element
.bp-doc .rangy-highlight {
background-color: #ff0;
background-color: lighten($highlight-yellow, 10%);
opacity: .25;
position: relative;
}

Expand Down
11 changes: 6 additions & 5 deletions src/components/ActionControls/ActionControls.scss
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@
margin: 0;
}

.ba-annotation-label ~ .ba-action-controls .ba-action-controls-draw,
.ba-annotation-label ~ .ba-action-controls .ba-action-controls-highlight {
.ba-annotator-label ~ .ba-action-controls .ba-action-controls-draw,
.ba-annotator-label ~ .ba-action-controls .ba-action-controls-highlight {
border-left: 1px solid $see-see;
margin-left: 5px;
}

// TABLET-SPECIFIC CSS
@media #{$tablet} {
@media #{$mobile}, #{$tablet} {
.ba-create-popover .ba-action-controls {
border: none;
box-shadow: none;
Expand All @@ -38,9 +38,10 @@
width: 100%;
}

.ba-annotation-label ~ .ba-action-controls .ba-action-controls-draw,
.ba-annotation-label ~ .ba-action-controls .ba-action-controls-highlight {
.ba-annotator-label ~ .ba-action-controls .ba-action-controls-draw,
.ba-annotator-label ~ .ba-action-controls .ba-action-controls-highlight {
border-left: 0;
margin: 0;
}
}

Expand Down
5 changes: 2 additions & 3 deletions src/components/AnnotationPopover/AnnotationPopover.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
import React from 'react';
import classNames from 'classnames';
import noop from 'lodash/noop';
import Overlay from 'box-react-ui/lib/components/flyout/Overlay';
import PlainButton from 'box-react-ui/lib/components/plain-button';
import IconClose from 'box-react-ui/lib/icons/general/IconClose';

import Internationalize from '../Internationalize';
import Overlay from './Overlay';
import CommentList from '../CommentList';
import { TYPES, CLASS_ANNOTATION_POPOVER } from '../../constants';

Expand Down Expand Up @@ -93,8 +93,7 @@ class AnnotationPopover extends React.PureComponent<Props> {
) : (
<span className='ba-popover-caret' />
)}

<Overlay className='ba-popover-overlay'>
<Overlay shouldDefaultFocus={!isMobile}>
{hasComments ? (
<CommentList comments={comments} onDelete={onDelete} />
) : (
Expand Down
2 changes: 1 addition & 1 deletion src/components/AnnotationPopover/AnnotatorLabel.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class AnnotatorLabel extends React.PureComponent<Props> {
const { id, isPending } = this.props;
return (
!isPending && (
<span className='ba-annotation-label'>
<span className='ba-annotator-label'>
<CommentText id={id} tagged_message={this.getAnnotatorLabelMessage()} translationEnabled={false} />
</span>
)
Expand Down
6 changes: 4 additions & 2 deletions src/components/AnnotationPopover/AnnotatorLabel.scss
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
@import '../../variables';

.ba {
.ba-annotation-label {
.ba-annotator-label {
line-height: 23px;
}

// MOBILE & TABLET CSS
@media #{$mobile}, #{$tablet} {
.ba-annotation-label {
.ba-annotator-label {
background: white;
border-bottom: 1px solid #ccc;
display: flex;
justify-content: center;
left: 0;
padding: 20px;
position: fixed;
Expand Down
23 changes: 23 additions & 0 deletions src/components/AnnotationPopover/Overlay.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// @flow
import * as React from 'react';

import OverlayComponent from 'box-react-ui/lib/components/flyout/Overlay';

type Props = {
shouldDefaultFocus: boolean,
children: React.Node
};

const Overlay = ({ shouldDefaultFocus, children }: Props) => {
if (shouldDefaultFocus) {
pramodsum marked this conversation as resolved.
Show resolved Hide resolved
return (
<OverlayComponent className='ba-popover-overlay' shouldOutlineFocus={false}>
{children}
</OverlayComponent>
);
}

return <div className='ba-popover-overlay'>{children}</div>;
};

export default Overlay;
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,26 @@ describe('components/AnnotationPopover', () => {
expect(wrapper.find('.ba-inline').length).toEqual(1);
});

test('should correctly render a BRUI Overlay if not on mobile', () => {
const wrapper = render({
canAnnotate: true,
comments,
isMobile: false
});
expect(wrapper).toMatchSnapshot();
expect(wrapper.find('Overlay').prop('shouldDefaultFocus')).toBeTruthy();
});

test('should correctly render a div without a Focus Trap if on mobile', () => {
const wrapper = render({
canAnnotate: true,
comments,
isMobile: true
});
expect(wrapper).toMatchSnapshot();
expect(wrapper.find('Overlay').prop('shouldDefaultFocus')).toBeFalsy();
});

test('should correctly render a popover with comments and reply textarea', () => {
const wrapper = render({
canAnnotate: true,
Expand Down
18 changes: 18 additions & 0 deletions src/components/AnnotationPopover/__tests__/Overlay-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import * as React from 'react';
import { shallow } from 'enzyme';

import Overlay from '../Overlay';

describe('components/Overlay', () => {
test('should correctly render a Focus Trap if not on mobile', () => {
const wrapper = shallow(<Overlay shouldDefaultFocus={true} />).dive();
expect(wrapper).toMatchSnapshot();
expect(wrapper.find('FocusTrap').length).toEqual(1);
});

test('should correctly render a div without a Focus Trap if on mobile', () => {
const wrapper = shallow(<Overlay shouldDefaultFocus={false} />);
expect(wrapper).toMatchSnapshot();
expect(wrapper.find('FocusTrap').length).toEqual(0);
});
});
Loading