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 11 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
hannahbergam marked this conversation as resolved.
Show resolved Hide resolved
};

constructor(props) {
super(props);

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

getContent() {
hannahbergam marked this conversation as resolved.
Show resolved Hide resolved
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.getContent()}</div>
</div>
);
}
}
41 changes: 41 additions & 0 deletions apps/src/code-studio/components/header/SignInCalloutWrapper.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
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

// The use of both session storage and cookies is to check for 1 day
// and 1 session, and display the callout again only once BOTH have passed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about why we need to put this value in session storage?

Also, is it possible to set the cookie for something like 20 hours? I'm thinking of the scenario where a student always does the activity at around the same time each day and wondering if the behavior might be weird if they do a level without seeing the popup and then see it on the next level. (Ideally I think we would want to reset this in the middle of the night each day.)

Finally, did we decide if we wanted to cap the number of times we show the callout (or just continue to show it once a day)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec stated that we wanted both, so if a student opens the same window the next day (example: student works on it at 2, and opens it again at 2:30 the next day), they don't see the message again in the same session. In fact, I think the use case for both is exactly what your example is actually covered by.

Even if the cookies flag expires, the student's session flag would still prevent it from being shown again unless they started a new lesson in a new window.

There was nothing stated in the spec about limiting the number of overall times it is shown, so I haven't included that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I think the common case might be that students close their browser and open a new one the next day (e.g. this is what they teach my kids to do when using school laptops) but we can start with this and refine it if we get feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I definitely see what you mean re: students are closing down entirely and starting fresh the next day. And I guess in that case, I'm not sure what the most desirable behavior is (to your point earlier: the solution is potentially a midnight refresh time, which isn't currently set up with either of these tools). I'll keep as-is for now, and be ready to strip out session storage/refine cookies if needed!

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);
this.state = {
hideCallout:
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;
});
});