Page MenuHomePhorge

Improving UX for ignoring timezone conflicts
AcceptedPublic

Authored by roberto.urbani on Aug 24 2023, 15:37.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Sep 24, 05:22
Unknown Object (File)
Thu, Sep 21, 02:00
Unknown Object (File)
Fri, Sep 15, 00:44
Unknown Object (File)
Sun, Sep 10, 11:30
Unknown Object (File)
Sun, Sep 10, 10:19
Unknown Object (File)
Fri, Sep 8, 11:16
Unknown Object (File)
Fri, Sep 8, 10:00
Unknown Object (File)
Fri, Sep 8, 06:11

Details

Summary

When there is a new timezone conflict, you will be able to ignore it with a checkbox.
Fix T15349

Preview:

timezone-dialog.png (271×819 px, 26 KB)

Test Plan

Having a conflicting timezone, click the notification so the usual popup appears. There is a checkbox, leave it checked to ignore the current conflict, uncheck to manually resolve the conflict by selecting one of the available timezones.

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25420_2
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 818
Build 818: arc lint + arc unit

Event Timeline

Adding another notification will only make this more annoying.

Why not just improve the current dialog to make it easier to ignore the conflict?

Adding another notification will only make this more annoying.

Why not just improve the current dialog to make it easier to ignore the conflict?

It would be one more click if we do it in the current dialog. The new notification is just one click and everything work; I'm for sure not a UX expert but this flow seems better to me

But it adds another notification, and now the user will have to pick between 2 buttons that say "solve conflict".
It's also not the common path - in the common case, we expect users to actually approve the timezone offered by the dialog (which should be the one detected from the browser).

Maybe we can improve the current popup from current:

Timezone Selection (including Ignore Conflict)
CancelChange Timezone

To (underlined the changed stuff):

Timezone Selection (without Ignore Conflict)
CancelIgnore ConflictChange Timezone

So, just a new button in the current popup

valerio.bozzolan retitled this revision from Improving UX for ignoring timezone conflicts Fix ref T15349 to Improving UX for ignoring timezone conflicts.Aug 24 2023, 18:37
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the test plan for this revision. (Show Details)

I kindly mark this as "request changes" since the author said that it needs an update to reflect the screenshot. Feel free to update with

arc diff --update D25420
This revision now requires changes to proceed.Aug 28 2023, 08:06

Updating D25420: Added a checkbox to ignore timezone conflict in Adjust Timezone dialog window. Using the checkbox instead of a button because this way it's possible to reuse the existing form. Also, i still didn't find a way to capture the additional button click event. This way, if you check the checkbox, it will use the current setting timezone.

Updating D25420: Updating D25420: fixed tests error

roberto.urbani edited the test plan for this revision. (Show Details)

if you check the checkbox, it will use the current setting timezone.

I don't understand what this means?

Also, can you post a screenshot of the dialog?

src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
102

Can probably just use phutil_tag('strong', array(), 'OR') here.

avivey requested changes to this revision.Aug 29 2023, 08:44
avivey added inline comments.
src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
108

Default to unchecked, and align the indentation.

This revision now requires changes to proceed.Aug 29 2023, 08:44
src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
102

Or maybe make "Or" be the label for the checkbox (would show up under "New Setting").

Updating D25420:

  • Fixed indentation.
  • Default unchecked checkbox
  • Using label + checkbox instead of the "Or" message alone
This revision is now accepted and ready to land.Aug 29 2023, 12:50
src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
16

@avivey Should we drop this line?

src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
16

🤷‍♀️

src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
16

xD

@roberto.urbani If you want, this is a nice moment for you to decide whenever to keep this option, or just set as an empty array.

Don't worry, in both cases someone out there in the planet will be furious. Feel free to roll a dice to decide.

I like this approach, and prefer it to my simpler attempt from D25426; but have some minor suggestions:

  • Instead of "You can ignore this conflict or adjust your profile setting to match your client", we should use the same order in the sentence as the options are shown to the user: "You can adjust your profile setting to match your client, or ignore this conflict."
  • The label for the checkbox should be just "Ignore Conflict", without the "Or" at the start.
  • When the "Ignore Conflict" checkbox is selected, the timezone dropdown should be disabled (and re-enabled if the checkbox is unmarked)

I like this approach, and prefer it to my simpler attempt from D25426; but have some minor suggestions:

  • Instead of "You can ignore this conflict or adjust your profile setting to match your client", we should use the same order in the sentence as the options are shown to the user: "You can adjust your profile setting to match your client, or ignore this conflict."
  • The label for the checkbox should be just "Ignore Conflict", without the "Or" at the start.
  • When the "Ignore Conflict" checkbox is selected, the timezone dropdown should be disabled (and re-enabled if the checkbox is unmarked)

I like these! Premising that the last part (hiding stuff) at the moment may be "somehow super-difficult" since at the moment this dialog is completely rendered by the server, and not by JavaScript.

So about this last part, maybe we can ignore at the moment, and create a sub-task for the future. But let's see other opinions.

(I accepted but before landing please first review all not-done inline comments: https://we.phorge.it/differential/revision/inlines/25420/ and this comment: https://we.phorge.it/D25420#12264 )

src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
88

Our style-guide says that if the call doesn't fit in a single line, then all arguments should be on individual lines:

$current_zone_formatted = phutil_tag(
  'strong',
  'array()',
  $current_zone_identifier);