Fix #274: Add UR study participation UI #278
Conversation
Notes: - I had to put the copy in brackets in StudyInvitation.jsx to be able to use the apostrophe it contains. This avoids the eslint rule error react/no-unescaped-entities. - The panels are wider than bryanbell's mocks, but the PR for mozilla#256 will fix that.
| * in the Study popup is clicked | ||
| */ | ||
| handleClickParticipate() { | ||
| browser.tabs.create({url: 'https://www.surveygizmo.com/s3/4693160/Price-Wise-Research-Study-Participant-Screener'}); |
There was a problem hiding this comment.
Should this be in the config?
| render() { | ||
| const {products} = this.props; | ||
| const {extractedProduct} = this.state; | ||
| const {extractedProduct, showStudyInvitation} = this.state; | ||
| if (showStudyInvitation) { |
There was a problem hiding this comment.
Can we add a config check here so that we can quick-disable this when it's time to stop recruiting? We can put up a PR changing the config default and just deploy that, and then remove the code itself later without the pressure.
There was a problem hiding this comment.
I added this feature flag check in the <StudyFooter /> component, which gates the showStudyInvitation value.
| @@ -49,6 +51,7 @@ export default class BrowserActionApp extends React.Component { | |||
| super(props); | |||
| this.state = { | |||
| extractedProduct: props.extractedProduct, | |||
| showStudyInvitation: false, | |||
There was a problem hiding this comment.
I think this is fine for this PR, but if we add any other "views" like this we should look into writing some sort of router or something that manages which view is "active". Otherwise figuring out which pages is active and the flow through pages will involve some complex if/then logic and several booleans like this.
There was a problem hiding this comment.
Can you share an example of what that might look like?
There was a problem hiding this comment.
Could be as simple as:
class BrowserActionApp {
constructor(props) {
super(props);
this.state = {
currentPage: 'productListing',
};
}
render() {
switch (this.state.currentPage) {
case 'productListing':
return <TrackedProductList />;
case 'studyEnrollment':
return <StudyEnrollment />;
}
}
}Or something as complex as React Router, depending on our needs.
| return ( | ||
| <button | ||
| type="button" | ||
| className="menu-item study" |
There was a problem hiding this comment.
These CSS classes are working out better than I expected.
| className="menu-item study" | ||
| onClick={this.props.onClick} | ||
| > | ||
| Help us improve Price Wise... |
There was a problem hiding this comment.
Consider using … here instead of three periods for the ellipsis.
There was a problem hiding this comment.
Why is this preferred?
There was a problem hiding this comment.
It makes the ellipsis a single character (which, semantically, it is), and also it is a visually different glyph vs three consecutive periods.
|
|
||
| /** Components for the StudyInvitation screen */ | ||
|
|
||
| .title-bar.study { |
There was a problem hiding this comment.
If we ever add a permanent screen with a layout this similar to the onboarding, we should factor out the styles to be reused between the two. For this temporary page, copying them here like you've done is a fine solution.
| <div className="study-invitation"> | ||
| <img className="hero" src={browser.extension.getURL('img/shopping-welcome.svg')} alt="" /> | ||
| <p className="description"> | ||
| {"Help us improve Price Wise by participating in a research study. Take this 5-minute survey to learn more about the study and see if you're a fit."} |
There was a problem hiding this comment.
Why the {""} surrounding the text?
There was a problem hiding this comment.
I had to put the copy in brackets in StudyInvitation.jsx to be able to use the apostrophe it contains. This avoids the eslint rule error react/no-unescaped-entities. Reference
There was a problem hiding this comment.
The HTML entity version of the apostrophe &apos is my preferred alternative, but it's an incredibly minor distinction.
Also incorporate feedback from Osmose, including the feature flag 'enableStudyUI' in config.js to turn on/off the UI.
@Osmose Ready for your review!
I'm awaiting two things still from other folks (pinged them this morning in slack; adding a DNM label for now):
Notes: