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

Switch to dark mode for the Museum Experience! #280

Merged
merged 40 commits into from Nov 11, 2019
Merged

Switch to dark mode for the Museum Experience! #280

merged 40 commits into from Nov 11, 2019

Conversation

@chelseatroy
Copy link
Member

chelseatroy commented Aug 6, 2019

  • Theme functions accept an attribute indicating whether this project is currently being displayed in a museum on this device (In dark museum exhibits, we want guests to see the tablet screens in dark mode rather than a jarring, white screen)
  • In a future commit, We'll key off of the role attribute in the project object (currently using project name as a placeholder).

Fixes # .

Invision Mock-ups:
https://projects.invisionapp.com/d/main/default/?origin=v7#/console/15642822/339630114/preview

Describe your changes.

Review Checklist

  • Does it work in Android and iOS?
  • Can you rm -rf node_modules/ && npm install and app works as expected?
  • Are tests passing?
- Theme functions accept an attribute indicating whether this project is currently being displayed in a museum on this device (In dark museum exhibits, we want guests to see the tablet screens in dark mode rather than a jarring, white screen)
- In a future commit, We'll key off of the role attribute in the project object (currently using project name as a placeholder).
@chelseatroy chelseatroy added the WIP label Aug 6, 2019
src/components/SideDrawer.js Outdated Show resolved Hide resolved
src/actions/projects.js Outdated Show resolved Hide resolved
src/actions/projects.js Outdated Show resolved Hide resolved
chelseatroy added 3 commits Sep 16, 2019
+ Also considered whether to push the museum mode logic down into NavBar.js component instead of settings at the classifierNavigator.js level. Decided, for now, to leave them at the classifierNavigator level because:

1. Only the classifier pages change in museum mode, but the NavBar settings are set all over the app.
2. Currently, the NavBar settings explicitly request the title, the presence of the hamburger menu, and the presence of back navigation...the three exact things that change in museum mode. This suggests that the NavBar settings represent a more generalized level of UI customizability than a specific 'mode.' Because of this, it is strange that the NavBar component ALSO has a setting for preview mode (in which the nav bar turns red). To make the levels of abstraction here consistent, we would want to switch out 'isPreview' in settings for 'color,' and set that color to red wherever we key off preview pages. (Another twist: 'isBeta' follows yet a third abstraction gradient with respect to the NavBar).
@chelseatroy chelseatroy marked this pull request as ready for review Sep 16, 2019
- Include propTypes validation for project id
- Change a typo back to the correct attribute
- Remove a deprecated attribute from SideDrawer instance
Copy link
Collaborator

wgranger left a comment

Most of the comments on code structure are simple items (camel vs. snake, removing unused props) however, there are a couple notes that would clean up the code a bit and perhaps catch a couple missed dark/light mode items. Giving the code a test run now to see if I missed anything.

}

function switchOn(mode, withModeStyle, withoutModeStyle) {
if (mode) {

This comment has been minimized.

Copy link
@wgranger

wgranger Sep 18, 2019

Collaborator

This can probably be a one-line ternary return mode ? withModeStyle : withoutModeStyle

This comment has been minimized.

Copy link
@chelseatroy

chelseatroy Oct 1, 2019

Author Member

It could indeed! And that's a common way to condense control flow, for sure.

The tradeoff with a ternary is that its brevity (and, in the case of JavaScript, its functionality) depends on executing a single operation—the return of the value—from each branch. So if I wanted to perform logging, printing, or debugging with a ternary conditional, I would first be forced to remove it from the ternary into a multi-line conditional like the one shown here. This function's relatively broad usage footprint (it gets executed in a lot of different places) makes it a likely place for a developer to want to debug if they have questions about how our conditional styling works, or want to understand whether or not a method that uses this method has the appropriate inputs if it's functioning in a way other than the developer expects. This already happened to me a few times because I passed my dark mode and light mode arguments to client functions in the wrong order :).

Because that's likely to happen a lot to this piece of code relative to others, it's important that I make this piece of code as easy to modify for debugging as possible. Leaving it as a multiline conditional facilitates that.

In fact, for me, the heuristic described above simplifies to the following rule of thumb: leave it multiline, rather than ternary, unless a multiline statement obfuscates additional information that a developer would otherwise be able to collect from the shape of the code they're looking at. So I'll use a ternary, say, in a variable assignment while writing JSX, because the shape of the JSX conveys a lot of meaning, and I want to keep my property assignments one-line if possible to convey that meaning successfully.

Sorry for noveling :)

return projects.map(project => {
return apiClient.type('project_roles')
.get({id: project.links.project_roles})
.then((roles) => {

This comment has been minimized.

Copy link
@wgranger

wgranger Sep 18, 2019

Collaborator

Can this return value become project_roles or something similar to avoid the roles.roles.includes statement on line 130 below?

This comment has been minimized.

Copy link
@chelseatroy

chelseatroy Oct 1, 2019

Author Member

Hmmm. Good point. This is kind of confusing.

The roles.roles thing comes from our API, but you bring up a good point. It might be possible for us to name these passed-in arguments in some way that stays true to the objects we are receiving while preventing some confusion.

This comment has been minimized.

Copy link
@wgranger

wgranger Nov 7, 2019

Collaborator

I was referring to line 129 where each item is referred to as role with the forEach iteration. We have control of that naming in the JS here. Something like projectRole would even work so it would be projectRole.roles.includes on the following line. This isn't breaking though, so I'm fine if you want to keep it role.roles. It just seemed odd to me.

@@ -121,4 +122,8 @@ ClassifierContainer.propTypes = {
})
}

ClassifierContainer.defaultProps = {
inMuseumMode: false

This comment has been minimized.

Copy link
@wgranger

wgranger Sep 18, 2019

Collaborator

Doesn't look like this container is using the inMuseumMode prop

This comment has been minimized.

Copy link
@chelseatroy

chelseatroy Oct 1, 2019

Author Member

Good catch! It wasn't before. Sadly, now, it is :( BUT you were right at the time!

)
import * as colorModes from '../../actions/colorModes'

export class NeedHelpButton extends Component {

This comment has been minimized.

Copy link
@wgranger

wgranger Sep 18, 2019

Collaborator

Is there a reason this was changed to a class component instead of a functional component?

This comment has been minimized.

Copy link
@chelseatroy

chelseatroy Oct 1, 2019

Author Member

Another good catch :) I don't have a firm reason. I have a mild preference for class components in React Native apps with dreams of community contribution because class components are more structurally similar to visual components in native Android and iOS apps, which broadens the pool of developers who don't have to code switch (no pun intended) to understand and modify our source code.

src/components/classifier/SwipeClassifier.js Outdated Show resolved Hide resolved
@@ -270,69 +296,69 @@ MultiAnswerClassifier.propTypes = {
subjectsSeenThisSession: PropTypes.array,
task: PropTypes.object,
answers: PropTypes.array,
classifierActions: PropTypes.any
classifierActions: PropTypes.any,
inMuseumMode: PropTypes.bool,

This comment has been minimized.

Copy link
@wgranger

wgranger Sep 18, 2019

Collaborator

Doesn't look like this prop is used by this component.

This comment has been minimized.

Copy link
@chelseatroy

chelseatroy Oct 1, 2019

Author Member

Good catch! It isn't. Will remove :)

},
tutorialContainer: {
flex: 1,
backgroundColor: 'white',

This comment has been minimized.

Copy link
@wgranger

wgranger Sep 18, 2019

Collaborator

Does this need to be a toggle for dark mode, using the colorModes function?

marginHorizontal: 25
},
backgroundView: {
backgroundColor: 'white',

This comment has been minimized.

Copy link
@wgranger

wgranger Sep 18, 2019

Collaborator

Same as the above note. Should you use colorModes here?

chelseatroy added 7 commits Sep 18, 2019
…r, which shows on many pages that do not contain a project.

WIP: Still need to determine how to:
- Set a default background color in the NavBar component for non-project pages
- Determine whether the other settings defaults in the NavBar component are working
- Make sure we only define $headerColor and $testRed in one place (rather than copy-pasting the rgba values)
…r comments from previous commit message.

At some point, we still should consider:
- Decoupling preview mode from the color of the safe area container
- remove references to $headerColor (which was the same color)
@chelseatroy

This comment has been minimized.

Copy link
Member Author

chelseatroy commented Oct 1, 2019

Hi @wgranger !

Another commit forthcoming with some changes apropos of the comments you have made. Thank you!

Becky and I did initial visual design review of the app together today. She has a few things that she would like to think about and get back to me on how she would like them done, so depending on how long that takes her, this branch might lie dormant for a bit until it's time to make those changes.

- Fix light mode, which did not have selection and deselection colors
- Add dark mode
- Add panel shading
chelseatroy added 6 commits Oct 20, 2019
- These attributes were double-setting some styling, so when we went to remove that styling, it made it confusing why they were still being set.
… light and dark mode

+ No disabled styling specified for dark mode. Will discuss with designer when she gets back in office, guessed in the meantime.
- Each button now has its own class with its own styling rules.
- This improves legibility; we're no longer overloading a button with a type attribute of 'answer' for buttons that aren't answer buttons.
- It also improves rendering speed; fewer of the buttons run a conditional to render, and those that do only contain a single conditional.
- The 'type' attribute SHOULD no longer be necessary. I am removing it in a separate commit so that, if the attribute is being used for something I didn't foresee and is difficult to pull out, I can reset easily.
TouchableOpacity,
View
StyleSheet,
TouchableOpacity,

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 22, 2019

'TouchableOpacity' is defined but never used no-unused-vars

This comment has been minimized.

Copy link
@wgranger

wgranger Nov 7, 2019

Collaborator

We can remove this

StyleSheet,
TouchableOpacity,
View
StyleSheet,

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 22, 2019

'StyleSheet' is defined but never used no-unused-vars

This comment has been minimized.

Copy link
@wgranger

wgranger Nov 7, 2019

Collaborator

Let's take this out

This comment has been minimized.

Copy link
@chelseatroy

chelseatroy Nov 11, 2019

Author Member

Removed

handlePress={this.props.finishTutorial}
additionalStyles={[styles.orangeButton]}
additionalTextStyles={[styles.blackText]}
text={'Let\s Go!'}/>

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 27, 2019

Unnecessary escape character: \s no-useless-escape

This comment has been minimized.

Copy link
@chelseatroy

chelseatroy Nov 11, 2019

Author Member

Removed

TouchableOpacity,
View
Platform,
ScrollView,

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 27, 2019

'ScrollView' is defined but never used no-unused-vars

This comment has been minimized.

Copy link
@chelseatroy

chelseatroy Nov 11, 2019

Author Member

Removed

handlePress={this.props.finishTutorial}
additionalStyles={[styles.orangeButton]}
additionalTextStyles={[styles.blackText]}
text={'Let\s Go!'}/>

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 27, 2019

Unnecessary escape character: \s no-useless-escape

This comment has been minimized.

Copy link
@chelseatroy

chelseatroy Nov 11, 2019

Author Member

Fixed

chelseatroy added 2 commits Nov 6, 2019
…de (they remain stacked in portrait mode)
componentWillReceiveProps(nextProps){
if ((this.props.isVisible !== nextProps.isVisible) && (nextProps.isVisible === true)){
this.open()
componentWillReceiveProps(nextProps) {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Nov 6, 2019

componentWillReceiveProps is deprecated since React 16.3.0, use UNSAFE_componentWillReceiveProps instead, see https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops react/no-deprecated

This comment has been minimized.

Copy link
@wgranger

wgranger Nov 7, 2019

Collaborator

Same as above

This comment has been minimized.

Copy link
@chelseatroy

chelseatroy Nov 11, 2019

Author Member

Updated!

onStartShouldSetPanResponderCapture: () => true,
onMoveShouldSetPanResponder: () => true,
onMoveShouldSetPanResponderCapture: () => true,
componentWillMount() {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Nov 6, 2019

componentWillMount is deprecated since React 16.3.0, use UNSAFE_componentWillMount instead, see https://reactjs.org/docs/react-component.html#unsafe_componentwillmount react/no-deprecated

This comment has been minimized.

Copy link
@wgranger

wgranger Nov 7, 2019

Collaborator

Let's heed the warning above and rename this to the recommendation above. I think by doing this the method should continue to work past version 17 instead of being ignored due to deprecation.

This comment has been minimized.

Copy link
@chelseatroy

chelseatroy Nov 11, 2019

Author Member

Good call. I ended up updating this and one other warning along the same lines.

One interesting thing I discovered while reading the documentation on these warnings is that most users don't want to use the suggested rename, but instead want to go with componentDidMount() here.

Figured I'd mention that in this comment in case anyone comes across this PR and is about to have to do something similar :). I wonder why react has chosen to recommend the deprecation namespace rename rather than switching to the new API.

@chelseatroy chelseatroy removed the WIP label Nov 7, 2019
Copy link
Collaborator

wgranger left a comment

Overall, the code here looks pretty good. I especially like how the buttons are extended in ClassifierButton.js. I think everything is pretty sound structure-wise (aside from the few in-line comments I've made).

A few items I noticed while running the app in museum mode.

  • In the drawing classifier, I am able to submit a blank classification without any rectangles drawn. Is this intentional? I suppose there could be a situation where there is nothing to mark in a subject, and a user would want to proceed without drawing a rectangle. Just want to make sure this is the intended functionality.

  • I can crash the app by selecting the "First Item" in the drawing workflow's tutorial (see image)

Screen Shot 2019-11-07 at 1 49 46 PM

  • In the swiping workflow, I can accidentally grab hold of the side menu if I grab the right most side of an image and try swiping to the left."

  • In the single-answer workflow, I cannot change my answer after selecting any of the other options.

If any of these issues will not affect museum-mode, I think it's fine to save them for another PR (given time constraints), but we need to open an issue for anything that isn't addressed in this PR.

@@ -1777,7 +1756,7 @@
);
PRODUCT_BUNDLE_IDENTIFIER = org.mobile.zooniverse;
PRODUCT_NAME = ZooniverseMobile;
PROVISIONING_PROFILE_SPECIFIER = "match Development org.mobile.zooniverse";
PROVISIONING_PROFILE_SPECIFIER = "Chelsea's Personal apparently Certificate ProvPro";

This comment has been minimized.

Copy link
@wgranger

wgranger Nov 7, 2019

Collaborator

This will restrict the bundle from being built or archived on any machine that doesn't have this provisioning profile. I think it is ok for now to get the app out the door, but it will cause some issues in the future if another developer attempts to archive the project and build an ipa for distribution.

return projects.map(project => {
return apiClient.type('project_roles')
.get({id: project.links.project_roles})
.then((roles) => {

This comment has been minimized.

Copy link
@wgranger

wgranger Nov 7, 2019

Collaborator

I was referring to line 129 where each item is referred to as role with the forEach iteration. We have control of that naming in the JS here. Something like projectRole would even work so it would be projectRole.roles.includes on the following line. This isn't breaking though, so I'm fine if you want to keep it role.roles. It just seemed odd to me.

@@ -57,7 +63,19 @@ class DrawingClassifier extends Component {
constructor(props) {
super(props)

const isPortrait = () => {

This comment has been minimized.

Copy link
@wgranger

wgranger Nov 7, 2019

Collaborator

This is a very handy method that I could see being pulled out to it's own helper at some point. However, it looks like it's only being used in this component for now. Nice and succinct. 👍

This comment has been minimized.

Copy link
@chelseatroy

chelseatroy Nov 11, 2019

Author Member

Most definitely :)

@@ -265,7 +265,7 @@ class ShapeEditorSvg extends Component {
}}
key={index}
{ ...shape }
blurred={this.state.shapeToRemoveIndex === index}
disabled={this.state.shapeToRemoveIndex === index}

This comment has been minimized.

Copy link
@wgranger

wgranger Nov 7, 2019

Collaborator

I don't believe EditableRect will recognize the disabled prop, unless I'm overlooking something.

onStartShouldSetPanResponderCapture: () => true,
onMoveShouldSetPanResponder: () => true,
onMoveShouldSetPanResponderCapture: () => true,
componentWillMount() {

This comment has been minimized.

Copy link
@wgranger

wgranger Nov 7, 2019

Collaborator

Let's heed the warning above and rename this to the recommendation above. I think by doing this the method should continue to work past version 17 instead of being ignored due to deprecation.

componentWillReceiveProps(nextProps){
if ((this.props.isVisible !== nextProps.isVisible) && (nextProps.isVisible === true)){
this.open()
componentWillReceiveProps(nextProps) {

This comment has been minimized.

Copy link
@wgranger

wgranger Nov 7, 2019

Collaborator

Same as above

StyleSheet,
TouchableOpacity,
View
StyleSheet,

This comment has been minimized.

Copy link
@wgranger

wgranger Nov 7, 2019

Collaborator

Let's take this out

TouchableOpacity,
View
StyleSheet,
TouchableOpacity,

This comment has been minimized.

Copy link
@wgranger

wgranger Nov 7, 2019

Collaborator

We can remove this

@chelseatroy

This comment has been minimized.

Copy link
Member Author

chelseatroy commented Nov 11, 2019

Overall, the code here looks pretty good. I especially like how the buttons are extended in ClassifierButton.js. I think everything is pretty sound structure-wise (aside from the few in-line comments I've made).

Thank you! I appreciate your validation on this choice :)

A few items I noticed while running the app in museum mode.

  • In the drawing classifier, I am able to submit a blank classification without any rectangles drawn. Is this intentional? I suppose there could be a situation where there is nothing to mark in a subject, and a user would want to proceed without drawing a rectangle. Just want to make sure this is the intended functionality.

Yes, I do believe this is intended behavior. I will talk to Laura and some of the research teams using the drawing classifier to ensure that this is the expected behavior.

  • I can crash the app by selecting the "First Item" in the drawing workflow's tutorial (see image)
Screen Shot 2019-11-07 at 1 49 46 PM

Yep! I was able to reproduce this. I was able to fix it by including an icon for the field guide item.

There are a few ways we can prevent this in the future:

  1. We can change the mobile app such that field guide items without icons still work.
  2. We can prevent projects from having field guide items without icons.

As I understand it, researchers and citizen scientists use the field guide to remind themselves of how things look that they are classifying in the project. So the utility of the field guide lies largely in the presence of a picture. For that reason, I think it would make sense to do #2.

We can, of course, also do #1, so I made an issue for it that we can prioritize as we see fit: #296

  • In the swiping workflow, I can accidentally grab hold of the side menu if I grab the right most side of an image and try swiping to the left."

Lol, you sure can! Successfully reproduced in citizen science mode. (In museum mode this will not happen because the side menu is not available there).

I made an issue for this: #297

  • In the single-answer workflow, I cannot change my answer after selecting any of the other options.

Correct! This is a known thing. Issue created: #298

If any of these issues will not affect museum-mode, I think it's fine to save them for another PR (given time constraints), but we need to open an issue for anything that isn't addressed in this PR.

@chelseatroy chelseatroy merged commit 2826033 into master Nov 11, 2019
3 checks passed
3 checks passed
Hound No violations found. Woof!
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@chelseatroy chelseatroy deleted the museum-mode branch Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.