-
Notifications
You must be signed in to change notification settings - Fork 314
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
Send mobile editor email #634
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
<?php | ||
|
||
namespace App\Notifications\Forms; | ||
|
||
use Illuminate\Bus\Queueable; | ||
use Illuminate\Contracts\Queue\ShouldQueue; | ||
use Illuminate\Notifications\Messages\MailMessage; | ||
use Illuminate\Notifications\Notification; | ||
|
||
class MobileEditorEmail extends Notification implements ShouldQueue | ||
{ | ||
use Queueable; | ||
|
||
protected $slug; | ||
|
||
/** | ||
* Create a new notification instance. | ||
* | ||
* @param string $slug | ||
* @return void | ||
*/ | ||
public function __construct($slug) | ||
{ | ||
$this->slug = $slug; | ||
} | ||
|
||
/** | ||
* Get the notification's delivery channels. | ||
* | ||
* @param mixed $notifiable | ||
* @return array | ||
*/ | ||
public function via($notifiable) | ||
{ | ||
return ['mail']; | ||
} | ||
|
||
/** | ||
* Get the mail representation of the notification. | ||
* | ||
* @param mixed $notifiable | ||
* @return \Illuminate\Notifications\Messages\MailMessage | ||
*/ | ||
public function toMail($notifiable) | ||
{ | ||
return (new MailMessage()) | ||
->subject('Continue editing your form on desktop') | ||
->line('We noticed you\'re editing a form on smaller screen.') | ||
->line('For the best form building experience, we recommend using a desktop computer.') | ||
->line('Ready to create something amazing? Click below to continue editing. 💻') | ||
->action(__('Continue Editing'), front_url('forms/' . $this->slug . '/edit')); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,6 +106,7 @@ import { captureException } from "@sentry/core" | |
import FormSettings from './form-components/FormSettings.vue' | ||
import FormEditorErrorHandler from '~/components/open/forms/components/FormEditorErrorHandler.vue' | ||
import { setFormDefaults } from '~/composables/forms/initForm.js' | ||
import { breakpointsTailwind, useBreakpoints } from '@vueuse/core' | ||
|
||
export default { | ||
name: "FormEditor", | ||
|
@@ -145,6 +146,16 @@ export default { | |
emits: ['mounted', 'on-save', 'openRegister', 'go-back', 'save-form'], | ||
|
||
setup() { | ||
// Check if the editor is visible on smaller screens then send an email | ||
const breakpoints = useBreakpoints(breakpointsTailwind) | ||
const isVisible = ref(breakpoints.smaller("md")) | ||
watch(isVisible, (newValue) => { | ||
if (newValue) { | ||
opnFetch('/open/forms/' + form.value.id + '/mobile-editor-email') | ||
} | ||
}) | ||
Comment on lines
+149
to
+156
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Several improvements needed for mobile detection logic. The current implementation has potential issues:
Consider this improved implementation: setup() {
+ const hasNotified = ref(false)
const breakpoints = useBreakpoints(breakpointsTailwind)
const isVisible = ref(breakpoints.smaller("md"))
- watch(isVisible, (newValue) => {
- if (newValue) {
- opnFetch('/open/forms/' + form.value.id + '/mobile-editor-email')
+ watch(isVisible, (newValue, oldValue) => {
+ // Skip initial trigger and check if already notified
+ if (oldValue === undefined || hasNotified.value || !form.value?.id) return
+
+ if (newValue) {
+ hasNotified.value = true
+ opnFetch('/open/forms/' + form.value.id + '/mobile-editor-email')
+ .catch(error => {
+ console.error('Failed to send mobile editor notification:', error)
+ })
}
- })
+ }, { flush: 'post' }) Additionally, consider using - const isVisible = ref(breakpoints.smaller("md"))
+ const isVisible = useDebouncedRef(breakpoints.smaller("md"), 300)
|
||
|
||
|
||
const { user } = storeToRefs(useAuthStore()) | ||
const formsStore = useFormsStore() | ||
const { content: form } = storeToRefs(useWorkingFormStore()) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the findings, I can now generate the final response:
Add rate limiting and error handling to protect email notifications
The review suggestions are valid as:
🔗 Analysis chain
Add rate limiting, error handling, and documentation.
While the basic implementation is functional, several improvements are recommended:
Consider applying these changes:
Additionally:
Let's verify the notification implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 163
Script:
Length of output: 131979
Script:
Length of output: 467
Script:
Length of output: 675