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

feat(create-meeting): custom url for new meetings #137

Merged
merged 10 commits into from
Jan 15, 2021

Conversation

jgoldhammer
Copy link
Collaborator

@jgoldhammer jgoldhammer commented Jan 6, 2021

Status

READY

Description

The PR adds the possibility for the user to use a custom url for new meetings.
The link is opened in the standard browser
If the link does not contain a http or https at the beginning, we will add the prefix.
If the link is empty or not valid, an alert is shown to the user.

Steps to Test or Reproduce

Go to preferences, services and choose custom url as option for create meeting.
Add a valid url and try to create the meeting via menubar or shortcut.
The link should be opened in the standard browser.

Delete the url from the preference and try to create the meeting.
An alert should be displayed with a hint.

Enter a not valid url in the preference and try to create a meeting.
An alert should be displayed with a hint.

- allow the user to choose a custom url in the preferences to create a new meeting. The url is opened in the default browser currently
@jgoldhammer jgoldhammer requested a review from leits January 6, 2021 19:48
@jgoldhammer jgoldhammer linked an issue Jan 6, 2021 that may be closed by this pull request
@jgoldhammer
Copy link
Collaborator Author

We should discuss if we should use a alert or we should use the notification. The problem with notifications is that the user does not allow the notification, so no notification is displayed in this case. Otherwise the notifications looks a little bit nicer.
What do you think, @leits ?

leits
leits previously approved these changes Jan 8, 2021
@leits leits self-requested a review January 8, 2021 08:41
@leits
Copy link
Owner

leits commented Jan 8, 2021

We should discuss if we should use a alert or we should use the notification. The problem with notifications is that the user does not allow the notification, so no notification is displayed in this case. Otherwise the notifications looks a little bit nicer.
What do you think, @leits ?

We send notifications when a user tries to open an event to which a link was not recognized.

I understand the meaning of alerts. But I think our communication should be consistent. And excluding notifications is a user decision that we must respect. If it's a DnD mode, then they will still be in the notification center.

@leits leits dismissed their stale review January 8, 2021 08:51

Rethink alerts idea

@jgoldhammer jgoldhammer added this to the 3.0 milestone Jan 8, 2021
@jgoldhammer
Copy link
Collaborator Author

We should discuss if we should use a alert or we should use the notification. The problem with notifications is that the user does not allow the notification, so no notification is displayed in this case. Otherwise the notifications looks a little bit nicer.
What do you think, @leits ?

We send notifications when a user tries to open an event to which a link was not recognized.

I understand the meaning of alerts. But I think our communication should be consistent. And excluding notifications is a user decision that we must respect. If it's a DnD mode, then they will still be in the notification center.

Yes, I fully understand. If an userbased action is not working, we have to display a hint to the user that something is not working. So I would vote for an dismissable alert.

@leits
Copy link
Owner

leits commented Jan 8, 2021

Do you mean NSAlert or alert style for notification?

@jgoldhammer
Copy link
Collaborator Author

Do you mean NSAlert or alert style for notification?

I would say NSAlert. A notification can be dismissed- the user must see that there is something wrong configured when he actively clicks something. Async notifications should stay the macos notifications IMHO.

Alternative 1: Check the link on entering in the preferences - this situation for alerting will never happen?!
Alternative 2: Maybe there are more smart notification libraries for macos.

@leits
Copy link
Owner

leits commented Jan 8, 2021

It makes sense.
What do you think about trying to join an event without a recognized meeting link?

About the alternatives. The first looks like a crutch and the second is out of the scope of this feature.

@jgoldhammer
Copy link
Collaborator Author

jgoldhammer commented Jan 8, 2021

It makes sense.
What do you think about trying to join an event without a recognized meeting link?

About the alternatives. The first looks like a crutch and the second is out of the scope of this feature.

Trying to join the next event is the same for me- the user actively wants to join, but there is no meeting link. If he has not activated the notifications, he will never see the error message.

Alternative 3: We could also check if the settings for notifications are switched off and than only show a NSAlert. When the notifications are enabled, we show a notification. WDYT?

@leits
Copy link
Owner

leits commented Jan 8, 2021

Hmm. Alternative 3 looks reasonable.
In your opinion, is the original implementation or alternative 3 the best option? Or do you think we need to keep looking for a solution?

- depending on the notification settings of the user, we will display an information via notifications or NSAlerts.
- when notifications are enabled, we will send macos notifications
- when notifications are disabled, we will send a NSAlert.

Testable with an empty custom url to create a new meeting or join the next event when there is no event to join
@jgoldhammer jgoldhammer requested a review from leits January 8, 2021 21:26
@jgoldhammer
Copy link
Collaborator Author

Hmm. Alternative 3 looks reasonable.
In your opinion, is the original implementation or alternative 3 the best option? Or do you think we need to keep looking for a solution?

I have implementation alternative 3. NsAlert will be only used when the user has switched off notifications...

I have simplified the url handling for the create new meeting feature and we will allow all kind of urls. The user must enter a valid url with a scheme, e.g. https

leits
leits previously approved these changes Jan 13, 2021
@leits
Copy link
Owner

leits commented Jan 15, 2021

@jgoldhammer, please, resolve the conflicts

…-new-meetings

# Conflicts:
#	MeetingBar.xcodeproj/project.pbxproj
#	MeetingBar/PreferencesView.swift
leits
leits previously approved these changes Jan 15, 2021
@leits leits merged commit c7f81b3 into leits:master Jan 15, 2021
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.

Allow account selection for new Google Meet meetings
2 participants