fix: [M3-8685] - Users unable to update timezone #11034
Conversation
| } | ||
|
|
||
| updateProfile({ timezone: String(timezoneValue) }).then(() => { |
There was a problem hiding this comment.
The is the cause of the bug. timezoneValue is an object and we "cast" it to a string, causing us to send {"timezone": "[Object object]"} to the API
| const label = formatOffset(tz); | ||
| return { label, value: tz.name }; | ||
| }); | ||
| .map((tz) => { |
There was a problem hiding this comment.
I was able to consolidate the two maps into one
| const minutes = (Math.abs(offset) % 60).toLocaleString(undefined, { | ||
| minimumIntegerDigits: 2, | ||
| useGrouping: false, | ||
| }); | ||
| const hours = Math.floor(Math.abs(offset) / 60); | ||
| const isPositive = Math.abs(offset) === offset ? '+' : '-'; | ||
|
|
||
| return `\(GMT ${isPositive}${hours}:${minutes}\) ${label}`; | ||
| return `(GMT ${isPositive}${hours}:${minutes}) ${label}`; |
There was a problem hiding this comment.
I don't think the \s are necessary. I removed them
| interface Timezone { | ||
| label: string; | ||
| name: string; | ||
| offset: number; | ||
| } | ||
|
|
||
| export interface TimezoneOption<T = string, L = string> { | ||
| label: L; | ||
| value: T; | ||
| } |
There was a problem hiding this comment.
We don't need these extra types. We can lean on TypeScript's inference to get a solution that is even more typesafe!
|
|
||
| if (!profile) { | ||
| return <CircleProgress />; | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| {loggedInAsCustomer ? ( | ||
| <StyledLoggedInAsCustomerNotice data-testid="admin-notice"> |
There was a problem hiding this comment.
Removed this one-off component in favor of a simple Notice just to clean things up a bit
| <StyledLoggedInAsCustomerNotice data-testid="admin-notice"> | ||
| <Typography variant="h2"> | ||
| While you are logged in as a customer, all times, dates, and graphs | ||
| will be displayed in your browser’s timezone ({timezone}). |
There was a problem hiding this comment.
This copy was just factually incorrect.
"all times, dates, and graphs will be displayed in your browser's timezone" has been corrected to say "all times, dates, and graphs will be displayed in the user's timezone "
| slotProps={{ | ||
| popper: { | ||
| sx: { | ||
| maxHeight: '285px', | ||
| overflow: 'hidden', | ||
| }, | ||
| }, | ||
| }} |
There was a problem hiding this comment.
I was able to clean up a lot of these one-off styles by using the fullWidth prop instead
| sx={{ width: '416px' }} | ||
| /> | ||
| </Stack> | ||
| <ActionsPanel |
There was a problem hiding this comment.
Replaced the ActionsPanel with a Button. I think a plain Button is more fitting here.
|
Coverage Report: ✅ |
9cc7cf8
to
b8e858b
Compare
| </> | ||
| ); | ||
| }; | ||
|
|
||
| const StyledRootContainer = styled('div', { | ||
| label: 'StyledRootContainer', | ||
| const TimezoneFormContainer = styled('div', { |
There was a problem hiding this comment.
It's a nit, but I think we should continue to name Styled Components with "Styled", it makes it so much easier for developers to quickly identify components that are purely meant to apply styles and and have no other function.
Doesn't need to be changed for this PR but something to consider.
There was a problem hiding this comment.
Ahh got it. I always thought that prefixing the component with "Styled" was kind of an anti-pattern but I see what you mean. Will keep this in mind going forward!
|
@bnussman-akamai the e2e failure appears to be relevant here
|
d7ce808
|
Thanks for catching that @abailly-akamai Should be fixed by d7ce808 I removed the |
|
all checks have passed - will be merging to staging! ty everyone 🚀 |
Description 📝
Preview 📷
Screen.Recording.2024-10-01.at.5.14.04.PM.mov
Screen.Recording.2024-10-01.at.5.12.59.PM.mov
How to test 🧪
As an Author I have considered 🤔