Details

Reviewers

valerio.bozzolan
avivey

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15349: Explicitly cover in "Timezone settings" dialog how to ignore the browser vs profile conflict

Commits

rP37ecdf233697: Improving UX for ignoring timezone conflicts

Summary

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

Fix T15349

Before	After

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

roberto.urbani created this revision.Aug 24 2023, 15:37

Owners added a reviewer: O1: Blessed Committers.Aug 24 2023, 15:37

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptAug 24 2023, 15:37

roberto.urbani requested review of this revision.Aug 24 2023, 15:37

Harbormaster completed remote builds in B806: Diff 1330.Aug 24 2023, 15:37

Adding another notification will only make this more annoying.

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

In D25420#11967, @avivey wrote:

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)
Cancel	Change Timezone

To (underlined the changed stuff):

Timezone Selection (without Ignore Conflict)
Cancel	Ignore Conflict	Change 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)

valerio.bozzolan added a task: T15349: Explicitly cover in "Timezone settings" dialog how to ignore the browser vs profile conflict.

valerio.bozzolan edited the summary of this revision. (Show Details)Aug 25 2023, 14:15

valerio.bozzolan attached a referenced file: F341415: adjust-timezone-dialog.png. (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.

Harbormaster completed remote builds in B815: Diff 1342.Aug 28 2023, 15:20

Updating D25420: Updating D25420: fixed tests error

Harbormaster completed remote builds in B816: Diff 1343.Aug 28 2023, 15:37

roberto.urbani edited the summary of this revision. (Show Details)Aug 28 2023, 15:38

roberto.urbani edited the summary of this revision. (Show Details)Aug 28 2023, 15:46

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?

avivey added inline comments.Aug 28 2023, 18:43

src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
105	Can probably just use `phutil_tag('strong', array(), 'OR')` here.

roberto.urbani edited the summary of this revision. (Show Details)Aug 29 2023, 07:07

avivey requested changes to this revision.Aug 29 2023, 08:44

avivey added inline comments.

src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
111	Default to unchecked, and align the indentation.

This revision now requires changes to proceed.Aug 29 2023, 08:44

avivey added inline comments.Aug 29 2023, 08:45

src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
105	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

Harbormaster completed remote builds in B817: Diff 1344.Aug 29 2023, 12:27

roberto.urbani marked 3 inline comments as done.Aug 29 2023, 12:31

roberto.urbani edited the summary of this revision. (Show Details)Aug 29 2023, 12:33

avivey accepted this revision.Aug 29 2023, 12:49

valerio.bozzolan resigned from this revision.Aug 29 2023, 12:50

This revision is now accepted and ready to land.Aug 29 2023, 12:50

cosmetic whitespace changes

Harbormaster completed remote builds in B818: Diff 1345.Aug 29 2023, 12:54

valerio.bozzolan added inline comments.Aug 29 2023, 12:57

src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
16	@avivey Should we drop this line?

avivey added inline comments.Aug 29 2023, 14:19

src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
16	🤷‍♀️

avivey mentioned this in D25426: Fix label of submit button in timezone conflict dialog.Aug 30 2023, 06:22

valerio.bozzolan added inline comments.Aug 30 2023, 06:40

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.

valerio.bozzolan mentioned this in Z5: Phorge (flame).Aug 30 2023, 06:53

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)

In D25420#12264, @waldyrious wrote:

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 )

avivey added inline comments.Aug 31 2023, 06:44

src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
93	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);

What about this small change? So, the checkbox has not a related "left" label, and has a more descriptive "right" label.

Ignore new setting.png (269×818 px, 28 KB)

If you want to try this, I can help amending this change. No problem.

valerio.bozzolan mentioned this in Z6: Phorge (Italian 🍕).Oct 23 2023, 20:37

Hoping to be useful, implement tips, to be easily tested.

https://we.phorge.it/D25420#12747

If you don't like this, say so, and I can revert for you.

If you like this final version, feel free to land.

Harbormaster completed remote builds in B873: Diff 1430.Oct 31 2023, 07:16

Hoping to be useful, I will land this after 2023-11-07 :) Thanks for this nice feature

Instead of adding a checkbox is there precedent for having a separate button alongside Cancel and Submit?

The checkbox is essentially a “Do not ask/warn me again” option which could potentially apply to many forms, however the behavior is slightly different. I think this current state could allow the form to get into a confusing state (select a value and click checkbox).

I think it would be more straightforward if instead of a checkbox it’s a new/different submit button. Or maybe the cancel button should just behave that way?

In D25420#12963, @speck wrote:

Instead of adding a checkbox is there precedent for having a separate button alongside Cancel and Submit?

Yeah interesting question. I remember I inspected AphrontDialogView without much success about that. I suppose it supports just Cancel or Confirm.

https://we.phorge.it/source/phorge/browse/master/src/view/AphrontDialogView.php

The checkbox is essentially a “Do not ask/warn me again” option which could potentially apply to many forms, however the behavior is slightly different. I think this current state could allow the form to get into a confusing state (select a value and click checkbox).

I think it would be more straightforward if instead of a checkbox it’s a new/different submit button. Or maybe the cancel button should just behave that way?

I've seen that the Cancel button usually is about just closing the Popup or going to another page without persisting anything. So at the moment we have:

Cancel → don't persist anything, the popup is still shown forever
Confirm → persist something. If you have the checkbox, it persist your preference to ignore this. You don't see the popup anymore.

If I understand correctly, you suggest to persist in both cases, so:

Ignore → persist to ignore the thing
Confirm → persist to select the thing

I have not a strong opinion about this. At the moment this may be a better compromise. Less "standard" (since the Cancel does not just close the popup) but indeed more intuitive to me

Closed by commit rP37ecdf233697: Improving UX for ignoring timezone conflicts. · Explain WhyNov 7 2023, 08:29

This revision was automatically updated to reflect the committed changes.

roberto.urbani added a commit: rP37ecdf233697: Improving UX for ignoring timezone conflicts.

Ah, thanks for landing

Any follow-up change is absolutely welcome to eventually implement any suggestion from here:

https://we.phorge.it/D25420#12963

valerio.bozzolan edited the summary of this revision. (Show Details)Nov 7 2023, 14:10

valerio.bozzolan mentioned this in Next Up.Nov 7 2023, 14:12

waldyrious mentioned this in D25458: Adjust message in timezone conflict form.Nov 7 2023, 15:59

waldyrious mentioned this in rP8092d90c79fc: Adjust message in timezone conflict form.Nov 10 2023, 09:03

avivey mentioned this in 2023 week 49.Dec 8 2023, 17:36