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

Hbergam/signin callout UI #38953

Merged
merged 13 commits into from
Feb 11, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions apps/i18n/common/en_us.json
Original file line number Diff line number Diff line change
Expand Up @@ -1224,6 +1224,8 @@
"notApplicable": "N/A",
"notCompleted": "Not completed",
"notSaved": "Not saved",
"notSignedInHeader": "You are not signed in",
"notSignedInBody": "You don't need an account to work on this lesson, but if you want to save your work, remember to sign in or create an account before you get started.",
"notStarted": "Not started",
"nPoints": "{numPoints, plural, one {1 point} other {# points}}",
"numMatchCorrect": "# match correct",
Expand Down
114 changes: 114 additions & 0 deletions apps/src/code-studio/components/header/SignInCallout.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import React from 'react';
import i18n from '@cdo/locale';
import PropTypes from 'prop-types';

const CALLOUT_COLOR = '#454545';
const TRIANGLE_BASE = 30;
const TRIANGLE_HEIGHT = 15;
const CALLOUT_Z_INDEX = 1040;
const CALLOUT_TOP = 30;

const styles = {
container: {
// The outermost div is relatively positioned so it can be used as a positional
// anchor for its children (which will be absolutely positioned to avoid affecting
// layout). This element must be 0-sized to avoid affecting layout.
position: 'relative',
height: 0,
width: 0
},
content: {
position: 'absolute',
top: CALLOUT_TOP,
right: -90,
zIndex: CALLOUT_Z_INDEX,
backgroundColor: CALLOUT_COLOR,
borderRadius: 3
},
modalBackdrop: {
// Most backdrop attributes come from the 'modal-backdrop' class defined by bootstrap
// but we need to override the opacity as the default opacity of 0.8 is too dark.
// Note that bootstrap defaults the z-index of the backdrop to 1040.
opacity: 0.5
},
upTriangle: {
position: 'absolute',
top: CALLOUT_TOP - TRIANGLE_HEIGHT,
left: -(TRIANGLE_HEIGHT / 2.0),
width: 0,
height: 0,
borderStyle: 'solid',
borderTopWidth: 0,
borderRightWidth: TRIANGLE_BASE,
borderBottomWidth: TRIANGLE_HEIGHT,
borderLeftWidth: TRIANGLE_BASE,
borderTopColor: 'transparent',
borderRightColor: 'transparent',
borderBottomColor: CALLOUT_COLOR,
borderLeftColor: 'transparent',
zIndex: CALLOUT_Z_INDEX
},
contentContainer: {
display: 'flex',
padding: 20
},
imageContainer: {
width: 100,
marginRight: 20
},
textContainer: {
width: 400,
textAlign: 'left',
whiteSpace: 'normal'
},
textHeader: {
marginTop: 0
}
};

/*
* This is a callout attached to the sign-in button that's used on CSF level
* pages to remind the user to sign-in. Note that the sign-in button is
* defined in shared/haml/user_header.haml and is not a React component.
* This component is injected into the page by src/code-studio/header.js.
*/
export default class SignInCallout extends React.Component {
static propTypes = {
handleClose: PropTypes.func.isRequired
};

constructor(props) {
super(props);

this.renderContent = this.renderContent.bind(this);
}

renderContent() {
return (
<div style={styles.contentContainer}>
<img
style={styles.imageContainer}
src="/shared/images/user-not-signed-in.png"
/>
<div style={styles.textContainer}>
<h2 style={styles.textHeader}>{i18n.notSignedInHeader()}</h2>
<p> {i18n.notSignedInBody()}</p>
</div>
</div>
);
}

render() {
return (
<div style={styles.container}>
<div
className="modal-backdrop"
style={styles.modalBackdrop}
onClick={this.props.handleClose}
/>
<div style={styles.upTriangle} />
<div style={styles.content}>{this.renderContent()}</div>
</div>
);
}
}
48 changes: 48 additions & 0 deletions apps/src/code-studio/components/header/SignInCalloutWrapper.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import React from 'react';
import cookies from 'js-cookie';
import SignInCallout from './SignInCallout';

const HideSignInCallout = 'hide_signin_callout';
hannahbergam marked this conversation as resolved.
Show resolved Hide resolved

// This class hold the display logic for the sign in callout, which prompts
// students to log in in order to save their progress. The callout's freqency
// is regulated here by the use of both session storage and cookies.
export default class SignInCalloutWrapper extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a header comment describing what this class does? (It's not immediately obvious from the name and I can't think of a better name.)

Copy link
Contributor

Choose a reason for hiding this comment

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

So today I came across all of these Dialog components, which is kind of similar to what we're building here. There's a BaseDialog which handles whether the dialog is displayed, we could name this component SignInCalloutDialog

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that there's some similarities, especially around the modal interactions, but the placement differences (anchored to an item vs. centered on the page) seems significant to me. My feeling is that we will eventually want a Callout component that's a peer of BaseDialog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment that hopefully can make it more clear! In regards to the comments about labeling this 'dialog'- it would seem more intuitive to me if the actual render/text component were labeled "dialog", as it's including what will be displayed (instead of the logic determining when it shows up). I'm definitely open to more discussion though.

constructor(props) {
super(props);
this.closeCallout = this.closeCallout.bind(this);
// Before the cookies are set, searching for them will return false, so the
// desired flag should logically read 'true' when set: resulting in the
// 'hide' name. Though this leads to a bit of a double negative in the
// display logic (if not hide: display), it is the clearest option for now.
this.state = {
hideCallout:
// The use of both session storage and cookies is to check for 1 day
// and 1 session, and display the callout again once BOTH have passed.
cookies.get(HideSignInCallout) === 'true' ||
sessionStorage.getItem(HideSignInCallout) === 'true'
};
}

closeCallout(event) {
this.setState({hideCallout: true});
cookies.set(HideSignInCallout, 'true', {expires: 1, path: '/'});
hannahbergam marked this conversation as resolved.
Show resolved Hide resolved
sessionStorage.setItem(HideSignInCallout, 'true');
event.preventDefault();
}

// For readibility: returning an empty div here explicitly if the callout is
// not supposed to be displayed. This avoids using a render statement that
// often returns *nothing.
render() {
if (this.state.hideCallout) {
return null;
} else {
return (
<div className="uitest-signincallout">
<SignInCallout handleClose={this.closeCallout} />
</div>
);
}
}
}
22 changes: 22 additions & 0 deletions apps/test/unit/code-studio/components/header/SignInCalloutTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import React from 'react';
import SignInCalloutWrapper from '@cdo/apps/code-studio/components/header/SignInCalloutWrapper';
import {shallow} from 'enzyme';
import {expect} from '../../../../util/reconfiguredChai';
import i18n from '@cdo/locale';

const wrapper = shallow(<SignInCalloutWrapper />);

describe('ViewPopup', () => {
hannahbergam marked this conversation as resolved.
Show resolved Hide resolved
it('displays the correct background darkness', () => {
wrapper.setState({hideCallout: false});
expect(wrapper.html().includes('opacity:0.5')).to.be.true;
});

it('shows the correct header', () => {
expect(wrapper.html().includes(i18n.notSignedInHeader())).to.be.true;
});

it('shows the correct image', () => {
expect(wrapper.html().includes('user-not-signed-in.png')).to.be.true;
});
});