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

Issue 456 auto play multi image #568

Merged
merged 19 commits into from
Jun 26, 2024
Merged

Conversation

coryzoon
Copy link
Contributor

Description of changes in order of the files listed below:

  • Added react-native-modal library for modal.
  • Pulls background from projects in order to display in classifier header.
  • Update the full screen media component to match designs.
  • Updated all classifier screens and any components that are part of the screens to match updated designs.
  • Remove different styling when in museum mode (should always be the same regardless of mode).
  • Updated animations to useNativeDrive: false, to remove warnings.
  • Added an auto play functionality for multi image classifications. Allows auto play, press and hold to pause, tap to pause, and updated pagination dots.
  • Created new components for buttons to support new designs.
  • Updated ClassificationPanel.js with the new tabs to match designs.
  • Updated Field Guide and Help modals to match new designs.
  • Updated styling for Tutorials to match new designs.
  • Fixed bug in SizedMarkdown.js where it would sometimes attempt to replace text when there is no text (very rare but worth fixing).
  • Updated SafeAreaContainer.js which is a container for the entire app that provides a safe area view with a background color. To match the designs I need to apply logic that would apply different backgrounds depending on if the screen was a classifier screen or not. Preview projects would still have the red background.
  • Added ClassifierHeader component for the new headers that would display the blurred project background image. Updated RootNavigator.js to not show the default header so that the custom header could be shown instead.

@@ -0,0 +1,51 @@
import React from 'react';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@@ -0,0 +1,58 @@
import React from 'react';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@@ -0,0 +1,181 @@
import React, { useState, useEffect, useRef } from 'react'; // Import useRef
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@@ -0,0 +1,66 @@
import React from 'react';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@@ -0,0 +1,58 @@
import React from 'react';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@@ -0,0 +1,142 @@
import React, { useState } from 'react';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@@ -0,0 +1,68 @@
import React from 'react';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@@ -0,0 +1,15 @@
import React from 'react';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@@ -0,0 +1,59 @@
import React from 'react';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@@ -0,0 +1,54 @@
import React from 'react';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@@ -0,0 +1,46 @@
import React from 'react';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@@ -0,0 +1,131 @@
import React from 'react';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

coryzoon added 7 commits May 9, 2024 08:50
…height for field guide to add more space. Add height to field guide item for items with no image. Replace expand icon with new icon for Android video expansion. Adjust help modal to only take up as much space as it needs rather than a full sized modal. Adjust text for the first time tutorial view to remove title and adjust styles.
…parent. Make markdown question centered when text doesn't take up fulls screen.
@coryzoon coryzoon requested a review from mcbouslog June 11, 2024 10:31
Copy link
Contributor

@mcbouslog mcbouslog left a comment

Choose a reason for hiding this comment

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

Great job on a lot of work! I've left some comments but I don't think there's anything that's blocking. The newer (functional) components are excellent, the older legacy components could use some clean up, but I think that's out of scope.

style={styles.zoonIconContainer}
>
<Image
source={require('../../images/zooni-nav-logo.png')}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not causing an issue, but typically see image imports as static at the top of a file, not sure if this could be.

@@ -3,16 +3,25 @@ import {
SafeAreaView,
View
} from 'react-native'
import { connect } from 'react-redux'
import { connect, useSelector } from 'react-redux'
Copy link
Contributor

Choose a reason for hiding this comment

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

If it'd take more than a minute don't bother, but if the mapStateToProps could be refactored/replaced with useSelector I think it would be preferable/consistent with newer react-redux.

import ButtonLarge from '../classifier/ButtonLarge'
import FieldGuideBtn from '../classifier/FieldGuideBtn'
import DrawingModeButton from './DrawingModeButton'
import ToolNameDrawCount from './ToolNameDrawCount'
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to these changes, but we've added an eventListener on line 68 and I don't think it's removed, for consideration remove the eventListener in componentWillUnmount.

<TouchableOpacity disabled={DeviceInfo.isTablet()} onPress={() => this.setState({isModalVisible: true})}
style={styles.subjectDisplayContainer}>
<TouchableOpacity
onPress={() => this.setState({ isModalVisible: true })}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider creating dedicated function similar to setQuestionVisibility to move out of render method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Disregard the moving state setting out of render comments, I think that's in a lot of the legacy code.

src/components/Markings/DrawingClassifier.js Show resolved Hide resolved
import PropTypes from 'prop-types';
import {addIndex, isEmpty, map} from 'ramda'
import {isEmpty} from 'ramda'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if ramda use can be replaced with lodash (added in PR 572)

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 now that ramda is used in a lot of different places in a handful of different ways and replacing ramda with lodash or vice versa is not trivial. Disregard related comments, but maybe add to a someday to look into (if there's overlap between the packages, replacing one with the other).


SwipeSingleImage.propTypes = {
uri: PropTypes.string,
onExpandButtonPressed: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onExpandButtonPressed: PropTypes.bool,
onExpandButtonPressed: PropTypes.func,

View,
Platform
} from 'react-native';
import { View, Platform, TouchableOpacity, TouchableWithoutFeedback } from 'react-native';
import PropTypes from 'prop-types';
import R from 'ramda';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can just import isEmpty, or refactor with lodash

onDisplayImageChange: PropTypes.func,
swiping: PropTypes.bool,
currentCard: PropTypes.bool,
onExpandButtonPressed: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onExpandButtonPressed: PropTypes.bool,
onExpandButtonPressed: PropTypes.func,

}
handleDimensionsChange() {
if (this.pager) {
setTimeout(() => this.pager.scrollTo({ y: 0 }), 300);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if needs to be cleared in componentWillUnmount?

@mcbouslog mcbouslog mentioned this pull request Jun 20, 2024
@coryzoon coryzoon merged commit 7111172 into master Jun 26, 2024
1 check passed
@coryzoon coryzoon deleted the issue-456-auto-play-multi-image branch September 3, 2024 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants