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

Support for long CRToast messages using kCRToastNotificationPreferredHeightKey and an image. #181

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ferrerod
Copy link

I attempted to use a CRToast with kCRToastNotificationTypeKey = CRToastTypeCustom and a custom height with kCRToastNotificationPreferredHeightKey as well as an alert image to fit a multiple-line message. I discovered a padding bug that would cause the padding around the alert image to grow unnecessarily wide as the height of the CRToast grew taller to fit the longer message. I tested these changes with the demo app and did see improvements there also.

@jadar
Copy link

jadar commented Aug 27, 2015

It'd be nice if there was a default padding, since it doesnt default to the notification height like it does now.

@dmiedema
Copy link
Contributor

This does seem to have some unintended side effects - just running it and showing the basic notification, the image is no longer inset the default amount. Also, how does this work with activity indicators? Or were activity indicators not an issue originally?

Also, it looked like when there was a lot of text & then image was right aligned there seemed to be no padding for the image view and the text.

I could totally see since the content view height creates the square for the image, that getting pretty out of hand, I'd never considered that. Although I think that would be an issue for activity indicators also so that might need to be accounted for.

@ferrerod
Copy link
Author

@dmiedema, I see the validation CI testImageFrameLeft/RightAlignment tests failed as you said...but these two tests only pass if the the height of the toast is used as the inset of the image. So a very tall toast makes for a huge amount of wasted space around a small image and the text ends up being a very narrow column. The current master branch, the taller the toast height, the uglier the layout becomes.
crtoast-taller-toast

@ferrerod
Copy link
Author

@dmiedema,I agree on your second point when the image is right aligned, the text can collide with the image depending on the word wrapping.

@ferrerod
Copy link
Author

@jadar would you suggest it defaults to the notification height unless the developer sets a preferred padding and in that case, use only preferred padding?

@dmiedema
Copy link
Contributor

Yeah, that layout is super bad when it gets too long. We should probably update the tests & add new ones for longer messages to make sure we don't regress with any future changes too.

Also, i think @jadar is right, unless a preferred padding is specified we should probably go with the default that way current look/functionality is maintained ¯_(ツ)_/¯

@jadar
Copy link

jadar commented Aug 27, 2015

Yes I think that's how I had it originally, but I can't remember for sure. It's probably the best course of action.

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.

3 participants