Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Allow user to set timezone #12775

Merged
merged 3 commits into from
Sep 2, 2024

Conversation

Timshel
Copy link
Contributor

@Timshel Timshel commented Jul 12, 2024

Hey,

This adds the ability for a user to set a Timezone which will replace the default one from the browser.

Worked on it mainly because Firefox with resist fingerprinting (Cf element-hq/element-web#25006) set the timezone to UTC, and I'm not aware of an easy way to whitelist only specific domains.

Edit: probably should have searched more before coding but there is supposed to be a whitelist in Firefox RFP but for now I have issues with it 😅. And this might be useful in other cases.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

@Timshel Timshel requested a review from a team as a code owner July 12, 2024 19:04
@Timshel Timshel requested review from t3chguy and florianduros July 12, 2024 19:04
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Jul 12, 2024
@Timshel Timshel marked this pull request as draft July 12, 2024 19:06
@t3chguy t3chguy requested a review from a team July 12, 2024 19:09
@t3chguy t3chguy marked this pull request as ready for review July 12, 2024 19:09
@t3chguy
Copy link
Member

t3chguy commented Jul 12, 2024

Sorry wrong button

@t3chguy t3chguy marked this pull request as draft July 12, 2024 19:09
@t3chguy t3chguy added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Jul 12, 2024
@Timshel
Copy link
Contributor Author

Timshel commented Jul 12, 2024

Not sure of the process but the PR is in a relative good state and working well.

Set it as WIP since:

  • I'm aware of a test failing since the properties screen now has more element.
  • Might need more translation ?

But wanted some feedback before spending more time on it :).

@t3chguy
Copy link
Member

t3chguy commented Jul 12, 2024

I've requested review from @matrix-org/product but they are quite busy so it may take some time, it is their decision what features land

@daniellekirkwood
Copy link

I'd like @americanrefugee 's input on the design of the button and it's layout. In terms of functionality I would accept this enhancement if/when implemented correctly

Here's how I see it in the Netlify build. Let me know if I'm actually looking at the wrong thing.
Screenshot 2024-07-16 at 12 35 19

@t3chguy t3chguy requested review from americanrefugee and removed request for a team July 16, 2024 12:11
Copy link

@americanrefugee americanrefugee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functionality of the Netlify screenshot if correct, but the design should look like this:
https://www.figma.com/design/qTWRfItpO3RdCjnTKPu4mL/Settings?node-id=97-15874&t=whVB5NajX00nqnN5-4

@Timshel
Copy link
Contributor Author

Timshel commented Jul 16, 2024

Made a short recording of the field in action:

Recording 2024-07-16 at 15 19 17

@Timshel
Copy link
Contributor Author

Timshel commented Jul 16, 2024

@americanrefugee I don't have access even with an account to the Figma file.

@americanrefugee
Copy link

Sorry @Timshel I didn't realize at first that you're not an Element employee - which is why I can't give you access to our design files.

However, here are screenshots of how the feature should look like, as well as dropdown menu states:
Dropdown
Settings_ General - PC

As for the functionality, I agree that it's useful to be able to type to filter dropdown menu results. However, the user shouldn't actually be typing anything into the field.

@t3chguy
Copy link
Member

t3chguy commented Jul 16, 2024

Such a Dropdown component is being worked on in element-hq/compound-web#204 and should be reusable here once it lands

@Timshel
Copy link
Contributor Author

Timshel commented Jul 16, 2024

Ok I'll switch to having the dropdown justified on the left with the label on top.

I agree that it's useful to be able to type to filter dropdown menu results. However, the user shouldn't actually be typing anything into the field.

I'll mention first that it's reusing the same dropdown that the one used at the top of the page for Application language.

But I'm not sure what you mean by not "typing anything into the field" do you mean not visible ?
The filtering is essentially a search, without the "search" term visible the user won't be able to see if he made a typo, and it will make deleting the search term quite tricky.

If you mean it to work like the classic list with keyboard navigation then I would mention that there is over 400 values so I believe filtering is quite important here.

@americanrefugee
Copy link

americanrefugee commented Jul 17, 2024

I suppose if the user starts typing then it can follow the regular form field component:

Type=Text, State=Typing, Dark=Off

And here is the Clear / X button:

Button_ Clear

  • By default, the selected menu item label should appear
  • If typing, the content are replaced with a cursor and "Clear" (X) button
    • The list should update as the user types
  • If the user clicks the x / clear button, the original label re-appears, the up arrow reappears, and all menu items are shown

Something like that?

@Timshel
Copy link
Contributor Author

Timshel commented Jul 17, 2024

@americanrefugee adding the Clear/ X would make sense, but I believe there is some "conflict" with the down arrow which indicate that it's a dropdown (probably why it's not already present).

As I mentioned I'm just reusing an existing component, so any change would impact multiple existing fields.
To be honest I believe this is a separate issue and should not be addressed here; and since this handling is already used in the same page I don't believe it should block this PR.

@americanrefugee
Copy link

Ok, reuse the existing component. Don't wanna block anything :)

@Timshel
Copy link
Contributor Author

Timshel commented Jul 17, 2024

@americanrefugee do you have a recommendation for the dropdown width ?

By default, it matches the width of the selected option (The container has the class mx_SettingsSubsection_contentStretch).
For the other dropdown (Application dropdown ) it matches the hint width (The app will reload after selecting another language).

Added the current Timezone in the Browser default placeholder/option using the Timezone short format defined as:

Short localized form (e.g.: PST, GMT-8)

Cf https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/DateTimeFormat

For now it look like this
element

@Timshel Timshel force-pushed the feature/userTimezone branch from 93b669d to ed26e22 Compare July 17, 2024 15:11
@Timshel
Copy link
Contributor Author

Timshel commented Aug 29, 2024

Managed to update the playwright snapshot even if the other test is failing with a timeout (the test was not reported as failing on the ci ...).

@Timshel
Copy link
Contributor Author

Timshel commented Aug 30, 2024

Hey sorry, with some jest test failing locally (on develop too) I did not look at why the playwright test started failing :(.
It appears I had unlinked matrix-react-sdk from my local element-web :(.

Hopefully this should be ok now, there is still the Typescript Syntax Check failing but no idea why, it's running without issue locally and the reported errors are clearly not limited to the change in this PR.

@florianduros
Copy link
Contributor

A js-sdk PR broke our CI, so don't worry about the current state of the ts errors.
I will take a look at the test, test it manually today or next week

@Timshel
Copy link
Contributor Author

Timshel commented Aug 30, 2024

@florianduros Just seen your latest push, I was thinking of adding to the playwright test (seen those default in other tests):

test.use({
  locale: 'en-GB',
  timezoneId: 'Europe/London',
});

This can be set globally too in the playwright.config.ts, but I believe it would be worth its own PR.

@Timshel Timshel force-pushed the feature/userTimezone branch from c4255fe to 41f50ac Compare August 30, 2024 15:18
@Timshel Timshel force-pushed the feature/userTimezone branch from 41f50ac to 084a157 Compare August 30, 2024 15:38
Copy link
Contributor

@florianduros florianduros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect it works!
Thanks for the contribution and not giving up! Well done!

@florianduros florianduros added this pull request to the merge queue Sep 2, 2024
@florianduros florianduros removed this pull request from the merge queue due to a manual request Sep 2, 2024
@florianduros florianduros added this pull request to the merge queue Sep 2, 2024
Merged via the queue into matrix-org:develop with commit ae15bbe Sep 2, 2024
26 checks passed
@Timshel Timshel deleted the feature/userTimezone branch September 2, 2024 09:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants