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

Split sheet from notify? #25

Closed
jeff-h opened this issue Jun 15, 2014 · 8 comments
Closed

Split sheet from notify? #25

jeff-h opened this issue Jun 15, 2014 · 8 comments

Comments

@jeff-h
Copy link
Member

jeff-h commented Jun 15, 2014

I wonder if it would be easier to find the sheet-creation ability if it were in its own top-level command? I didn't expect to find it on the Notify command.

i.e.

MacGap.Sheet(...);

rather than

MacGap.Notice.notify({
    'type' : 'sheet',
    ...
});
@rawcreative
Copy link
Member

It’s not a configurable sheet, it’s only a setting to use the sheet type of notification (NSAlert) rather than the notification center..

On Jun 15, 2014, at 7:46 PM, jeff-h notifications@github.com wrote:

I wonder if it would be easier to find the sheet-creation ability if it were in its own top-level command? I didn't expect to find it on the Notify command.

i.e.

MacGap.Sheet(...);
rather than

MacGap.Notice.nofify({
'type' : 'sheet',
...
});

Reply to this email directly or view it on GitHub.

@jeff-h
Copy link
Member Author

jeff-h commented Jun 16, 2014

Ah, I see. I still don't feel the connection to notification centre is very strong. Could we provide an optional arg to our implementation of the JavaScript alert() command perhaps?

Also, in my documentation creation, I wasn't able to work out how to actually call the notification stuff. Would you mind giving me a tiny example when you have a moment?

@perifer
Copy link
Contributor

perifer commented Jun 16, 2014

@jeff-h, you mean displaying a notification? This worked for me:

MacGap.hide(); // Hide since notification popup only show if the application is in background.
MacGap.Notify.notify({title: 'A title', content: 'Some content.'});

MacGap.unhide() didn't seem to work though (which would be logical to use in an example).

@jeff-h
Copy link
Member Author

jeff-h commented Jun 16, 2014

Weird, thanks for that — it looks exactly like what I tried, but your code works for me. Not sure what I had wrong.

MacGap.Notify.notify({
    type: 'sheet',
    title: 'A title',
    content: 'Some content.'
});

...also works. I still feel this API should somehow be tied in with alert();

Also, the double notify i.e. Notify.notify() seems redundant, although I understand why it's there from an Obj-C point of view. Is there any way around it?

@rawcreative
Copy link
Member

There's no need to hide the app to get the notification to pop up, the notice command in MacGap1 did not include to the code to allow notifications to always show, so they only were shown when the app was hidden.

As for moving the sheet to the js alert() function, you can't pass arguments to the alert() function, only the alert message that is to be displayed. This means that while it's possible to show a sheet instead of the regular alert pop-up, you can't do both, it's either one or the other for all instances of alert().

The Notify command is meant to be a means to handle notifying users of something, there's multiple ways to do that in an app, so I don't see why it would need to solely be for notification center messages. As for the notify() method itself, yeah it's redundant, since the class only has one command, it can just be moved to the App class and be part of the MG object itself.

@jeff-h
Copy link
Member Author

jeff-h commented Jun 16, 2014

Thanks for clarifying things — I've tidied the docs accordingly.

Re: the redundant notify() method, do you think we may end up adding more kinds of notifications that would need their own command? I can't think of any particularly, and since you're accepting a dictionary as the arg and are already switching on type it looks pretty safe to roll into the App command. However, I'm certainly happy to leave it as-is if you think it's better long-term.

@rawcreative
Copy link
Member

I added a notify method to the App command yesterday, along with a sound flag to disable the sound if desired, default is on. I left the notify command in place temporarily just for backwards compat so it wasn't a breaking change immediately, but note it in the docs that Notify has been deprecated and will be removed and to use MacGap.notify() instead.

@jeff-h
Copy link
Member Author

jeff-h commented Jun 17, 2014

Thanks — docs updated.

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

No branches or pull requests

3 participants