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

[5.3] Add On-Ramp Aggregator What's new modal #4484

Merged

Conversation

wachunei
Copy link
Member

@wachunei wachunei commented Jun 9, 2022

⚠️ This PR Depends on #4479

This PR adds the content of the On Ramp Aggregator What's new modal

  • Title
  • Description
  • Image
  • CTA Label

This PR also improves:

  • Touchable area of pages is maximized
  • Pages indicators are touchable
  • Hides indicators and disable horizontal scrolling when there is only one (1) page
  • Improves version typing to be correct

Comment on lines +54 to +60
},
slideItemContainerContent: {
paddingBottom: 16,
flexGrow: 1,
},
slide: {
flex: 1,
Copy link
Member Author

Choose a reason for hiding this comment

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

This allows the slide to be full height

@@ -119,11 +125,11 @@ const createStyles = (colors) =>
});

const WhatsNewModal = (props) => {
const scrollViewRef = useRef();
Copy link
Member Author

Choose a reason for hiding this comment

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

This ref is later used to scroll when indicators are pressed.

const [featuresToShow, setFeaturesToShow] = useState(null);
const [show, setShow] = useState(false);
const routes = useNavigationState((state) => state.routes);
const slideIds = [0, 1];
const [currentSlide, setCurrentSlide] = useState(slideIds[0]);
const [currentSlide, setCurrentSlide] = useState(0);
Copy link
Member Author

Choose a reason for hiding this comment

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

slides where fixed to be 2, this is now agnostic of the length of the slides coming from whatsNew

Comment on lines +245 to +251
<ScrollView
key={key}
style={styles.slideItemContainer}
contentContainerStyle={styles.slideItemContainerContent}
>
<TouchableWithoutFeedback>
<View>
<View style={styles.slide}>
Copy link
Member Author

Choose a reason for hiding this comment

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

This allows slides to be full height

@@ -280,9 +290,11 @@ const WhatsNewModal = (props) => {
</TouchableOpacity>
</View>
</View>
{featuresToShow && (
{whatsNew.slides.length > 0 && (
Copy link
Member Author

Choose a reason for hiding this comment

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

Conditionally rendering based on featuresToShow was causing a content flash when CTAs were pressed.

<View style={styles.slideContent}>
<ScrollView
ref={scrollViewRef}
scrollEnabled={whatsNew.slides.length > 1}
Copy link
Member Author

@wachunei wachunei Jun 9, 2022

Choose a reason for hiding this comment

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

This ref is used to scroll then indicators are pressed. Scroll is disabled if there's only one page.

Comment on lines 307 to 330
{whatsNew.slides.length > 1 && (
<View style={styles.progessContainer}>
{whatsNew.slides.map((_, index) => (
<TouchableWithoutFeedback
key={index}
onPress={() => {
scrollViewRef?.current?.scrollTo({
y: 0,
x: index * slideItemWidth,
});
setCurrentSlide(index);
}}
hitSlop={{ top: 10, left: 10, bottom: 10, right: 10 }}
>
<View
style={[
styles.slideCircle,
currentSlide === index && styles.slideSolidCircle,
]}
/>
</TouchableWithoutFeedback>
))}
</View>
)}
Copy link
Member Author

Choose a reason for hiding this comment

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

Indicators are shown only when there are more than 1 pages and they are pressable.

@wachunei wachunei marked this pull request as ready for review June 9, 2022 21:23
@wachunei wachunei requested a review from a team as a code owner June 9, 2022 21:23
@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2022

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Made a small adjustment. LGTM

@wachunei
Copy link
Member Author

wachunei commented Jun 9, 2022

Should I flip the onlyUpdates property 🤔 ? I did

@mobularay mobularay changed the title Add On-Ramp Aggregator What's new modal [5.3] Add On-Ramp Aggregator What's new modal Jun 10, 2022
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Forgot to submit review, left comment!

@@ -5,7 +5,7 @@ import { WhatsNew } from './types';

export const whatsNew: WhatsNew = {
// All users that have <1.0.7 and are updating to >=1.0.7 should see
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update this to either use the property names or what the actual versions are. In this case, both would be 5.3.0

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in b63591f

Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

@wachunei wachunei merged commit 6de2b2b into feature/4479-update-whats-new-modal Jun 13, 2022
@wachunei wachunei deleted the feature/onramp-whats-new-modal branch June 13, 2022 16:29
@github-actions github-actions bot locked and limited conversation to collaborators Jun 13, 2022
@mobularay mobularay added the release-5.3.0 Issue or pull request that will be included in release 5.3.0 label Jun 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-5.3.0 Issue or pull request that will be included in release 5.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants