Skip to main content
Code Review

Return to Answer

Removed blockquotes around code??, changed edits I disagree with, but kept majority.
Source Link
Pebbl
  • 161
  • 3
  • 11

If it were me I'd take your generalised code for handling your date, repeat and time fields and abstract it away behind your own form widget, something like:

 +------------------+
Summary text about repeat date and time | click to change |
 +------------------+ 
 +------------------+
 Summary text about repeat date and time | click to change |
 +------------------+ 
Updates:
 +------------------+
Repeats weekly, every Tuesday, at 18:00 and 20:00 | click to change |
 +------------------+
Reminders:
 +--------------+
/no reminders set/ | click to set |
 +--------------+
[ ] tick to synchronize Reminders with Updates
 Updates:
 +------------------+
 Repeats weekly, every Tuesday, at 18:00 and 20:00 | click to change |
 +------------------+
 Reminders:
 +--------------+
 /no reminders set/ | click to set |
 +--------------+
 [ ] tick to synchronize Reminders with Updates

Once you have that kind of layout, you'd just need to rework your code so that using either widget calls into existence your generalised GUI for setting a date, frequency and time - you could use a popover, or a new admin screen. By tidying this code away in a reusable widget you'd probably find places where you could abstract and cut down code (i.e. not having to be specific with class names or input synchronizationsynchronisation).

Doing things in the manner above would also help with extensibility, if you ever needed to add an extra type of event - or wanted to allow the user to define multiple update events. You would be able to add a new widget easily.

Anyway, apologies if this wasn't the answer you were looking for, as I said the code to me looks pretty good for what you have - it's more the approach that could improve things.

If it were me I'd take your generalised code for handling your date, repeat and time fields and abstract it away behind your own form widget:

 +------------------+
Summary text about repeat date and time | click to change |
 +------------------+ 
Updates:
 +------------------+
Repeats weekly, every Tuesday, at 18:00 and 20:00 | click to change |
 +------------------+
Reminders:
 +--------------+
/no reminders set/ | click to set |
 +--------------+
[ ] tick to synchronize Reminders with Updates

Once you have that kind of layout, you'd just need to rework your code so that using either widget calls into existence your generalised GUI for setting a date, frequency and time - you could use a popover, or a new admin screen. By tidying this code away in a reusable widget you'd probably find places where you could abstract and cut down code (i.e. not having to be specific with class names or input synchronization).

Doing things in the manner above would also help with extensibility, if you ever needed to add an extra type of event - or wanted to allow the user to define multiple update events. You would be able to add a new widget easily.

Anyway, apologies if this wasn't the answer you were looking for, as I said the code to me looks pretty good for what you have - it's more the approach that could improve things.

If it were me I'd take your generalised code for handling your date, repeat and time fields and abstract it away behind your own form widget, something like:

 +------------------+
 Summary text about repeat date and time | click to change |
 +------------------+ 
 Updates:
 +------------------+
 Repeats weekly, every Tuesday, at 18:00 and 20:00 | click to change |
 +------------------+
 Reminders:
 +--------------+
 /no reminders set/ | click to set |
 +--------------+
 [ ] tick to synchronize Reminders with Updates

Once you have that kind of layout, you'd just need to rework your code so that using either widget calls into existence your generalised GUI for setting a date, frequency and time - you could use a popover, or a new admin screen. By tidying this code away in a reusable widget you'd probably find places where you could abstract and cut down code (i.e. not having to be specific with class names or input synchronisation).

Doing things in the manner above would also help with extensibility, if you ever needed to add an extra type of event or wanted to allow the user to define multiple update events. You would be able to add a new widget easily.

Anyway, apologies if this wasn't the answer you were looking for, as I said the code to me looks pretty good for what you have it's more the approach that could improve things.

added 55 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238

Considering the complexity of your guiGUI I'd say your code is already quite trim. If you wish to cut it down I'd go about simplifying the user interface, personally I found it confusing - but then again I am using it out of context.

Whilst it's great from a code view point to reuse interfaces (i.e the duplication between update and reminder sections) - from a user povPOV this can be confusing when the repeatitionrepetition appears constantly.

If it were me I'd take your generalised code for handling your date, repeat and time fields and abstract it away behind your own form widget. Something like:

 +------------------+
Summary text about repeat date and time | click to change |
 +------------------+
 +------------------+
Summary text about repeat date and time | click to change |
 +------------------+ 
Updates:
 +------------------+
Repeats weekly, every tuesday, at 18:00 and 20:00 | click to change |
 +------------------+
Reminders:
 +--------------+
/no reminders set/ | click to set |
 +--------------+
[ ] tick to syncronise Reminders with Updates
Updates:
 +------------------+
Repeats weekly, every Tuesday, at 18:00 and 20:00 | click to change |
 +------------------+
Reminders:
 +--------------+
/no reminders set/ | click to set |
 +--------------+
[ ] tick to synchronize Reminders with Updates

Once you have that kind of layout, you'd just need to rework your code so that using either widget calls into existence your generalised guiGUI for setting a date, frequency and time - you could use a popover, or a new admin screen. By tidying this code away in a reusable widget you'd probably find places where you could abstract and cut down code (i.e. not having to be specific with class names or input syncronisationsynchronization).

{repeats:'weekly',every:'tuesday','at':[18:00,20:00]}
{repeats:'weekly',every:'tuesday','at':[18:00,20:00]}

Considering the complexity of your gui I'd say your code is already quite trim. If you wish to cut it down I'd go about simplifying the user interface, personally I found it confusing - but then again I am using it out of context.

Whilst it's great from a code view point to reuse interfaces (i.e the duplication between update and reminder sections) - from a user pov this can be confusing when the repeatition appears constantly.

If it were me I'd take your generalised code for handling your date, repeat and time fields and abstract it away behind your own form widget. Something like:

 +------------------+
Summary text about repeat date and time | click to change |
 +------------------+
Updates:
 +------------------+
Repeats weekly, every tuesday, at 18:00 and 20:00 | click to change |
 +------------------+
Reminders:
 +--------------+
/no reminders set/ | click to set |
 +--------------+
[ ] tick to syncronise Reminders with Updates

Once you have that kind of layout, you'd just need to rework your code so that using either widget calls into existence your generalised gui for setting a date, frequency and time - you could use a popover, or a new admin screen. By tidying this code away in a reusable widget you'd probably find places where you could abstract and cut down code (i.e. not having to be specific with class names or input syncronisation).

{repeats:'weekly',every:'tuesday','at':[18:00,20:00]}

Considering the complexity of your GUI I'd say your code is already quite trim. If you wish to cut it down I'd go about simplifying the user interface, personally I found it confusing - but then again I am using it out of context.

Whilst it's great from a code view point to reuse interfaces (i.e the duplication between update and reminder sections) - from a user POV this can be confusing when the repetition appears constantly.

If it were me I'd take your generalised code for handling your date, repeat and time fields and abstract it away behind your own form widget:

 +------------------+
Summary text about repeat date and time | click to change |
 +------------------+ 
Updates:
 +------------------+
Repeats weekly, every Tuesday, at 18:00 and 20:00 | click to change |
 +------------------+
Reminders:
 +--------------+
/no reminders set/ | click to set |
 +--------------+
[ ] tick to synchronize Reminders with Updates

Once you have that kind of layout, you'd just need to rework your code so that using either widget calls into existence your generalised GUI for setting a date, frequency and time - you could use a popover, or a new admin screen. By tidying this code away in a reusable widget you'd probably find places where you could abstract and cut down code (i.e. not having to be specific with class names or input synchronization).

{repeats:'weekly',every:'tuesday','at':[18:00,20:00]}
simplified
Source Link
Pebbl
  • 161
  • 3
  • 11

IfAlso, if you developed a interpretable and generalised way of describing a repeat event, like:

This could be stored in a hidden field or attribute along with the widget once used. With this, syncronising the information between widgets suddenly becomes quite easy, wouldn't require specific fields to be constantly harmonised, and any harmonising work could be treated as it's own abstracted separatedone in a simple function outside of all the widget code.

If you developed a interpretable and generalised way of describing a repeat event, like:

This could be stored in a hidden field or attribute along with the widget once used. With this, syncronising the information between widgets suddenly becomes quite easy, wouldn't require specific fields to be constantly harmonised, and could be treated as it's own abstracted separate function.

Also, if you developed a interpretable and generalised way of describing a repeat event, like:

This could be stored in a hidden field or attribute along with the widget once used. With this, syncronising the information between widgets suddenly becomes quite easy, wouldn't require specific fields to be constantly harmonised, and any harmonising work could be done in a simple function outside of all the widget code.

Source Link
Pebbl
  • 161
  • 3
  • 11
Loading
default

AltStyle によって変換されたページ (->オリジナル) /