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

Low volume spacing fix #41

Merged
merged 33 commits into from
Jun 24, 2018

Conversation

alex-taffe
Copy link
Collaborator

@alex-taffe alex-taffe commented May 23, 2018

This includes changes from #38 and should not be merged until then

Also, I removed MPGNotification from the actual project and changed it to a cocoa pod. The newer version has support for iPhone X so that was most of our issue, but it’s also just good to get it out of the project. You’ll need to run a pod update; pod install before this compiles most likely

Fixes #26

Ross-Gibson and others added 28 commits May 21, 2018 09:20
…rtrap-ios into compiler-warning-cleanups

# Conflicts:
#	TriggertrapSLR/TimelapseProViewController.swift
Copy link
Collaborator

@Ross-Gibson Ross-Gibson left a comment

Choose a reason for hiding this comment

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

@alex-taffe this looks good. Just the one comment in the code.

Also, I've noticed that these changes cause the title in the notification to become left-aligned rather than entered, and it looks like the weight of the font has changed too. Are we able to use the original design?

self.notification?.duration = 2.0
self.notification?.show()
//this needs to be reinstantiated each time to avoid glitchy behavior (sadly)
self.notification = MPGNotification(title: NSLocalizedString("Low Volume", comment: "Low Volume"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alex-taffe can you explain a bit more the reasoning for this? What exactly is the "glitchy behaviour" and how does this resolve it?

Also, why have we removed the check for isAnimating, will that not cause multiple notifications to stack on-top of each other?

@alex-taffe
Copy link
Collaborator Author

@Ross-Gibson without the reinstantiation I get this weird bug. I’m not really sure why and I've messed around with the threading for awhile to see if that’s the issue, but I honestly don't know
YAXP2849.MP4.zip

As for the is animating, that property does not exist anymore in the library (as of the iPhone X fix). There is a dismissHandler property on the notification object, but the library is written in Objective C so of course it’s not playing super nice with swift. I’ll play around with it more to get it set up to avoid stacking.

As for the design, also part of the library change. Lemme look into it more

@alex-taffe
Copy link
Collaborator Author

@Ross-Gibson I got everything fixed minus the instantiation problem, which I have no idea how to fix

@Ross-Gibson Ross-Gibson merged commit 9c10c9b into Triggertrap:develop Jun 24, 2018
@alex-taffe alex-taffe deleted the low-volume-spacing-fix branch June 24, 2018 16:40
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.

None yet

2 participants