Skip to content

Feat/3334 show times in timezones for bookingpage#4971

Merged
PeerRich merged 22 commits intomainfrom
feat/3334-show-times-in-timezones-for-bookingpage
Oct 19, 2022
Merged

Feat/3334 show times in timezones for bookingpage#4971
PeerRich merged 22 commits intomainfrom
feat/3334-show-times-in-timezones-for-bookingpage

Conversation

@JeroenReumkens
Copy link
Copy Markdown
Contributor

@JeroenReumkens JeroenReumkens commented Oct 12, 2022

What does this PR do?

Adds a popup/tooltip to view the meeting time in the different timezones of all attendees.

Behaviour:

  • Only shows unique timezones, doesn't show duplicates
  • If everyone is in the same timezone popup won't show
  • Timezones are sorted by their GMT timezone (earliest to latest)
  • If someone meets a day before / after we show -1 / +1
  • As discussed with Ciaran we don't show avatars for now

Fixes #3334

https://www.loom.com/share/2caee7c38aa74a588f83ccec9af5672b

Environment: Staging(main branch)

Type of change

  • New feature (non-breaking change which adds functionality)

How should this be tested?

  • Create different meetings and verify that the popup shows the correct times in the respective timezones.

@vercel
Copy link
Copy Markdown

vercel Bot commented Oct 12, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Oct 19, 2022 at 9:43AM (UTC)

@CarinaWolli
Copy link
Copy Markdown
Member

@JeroenReumkens I just tested it and it shows +1, even when it is the same day:
Screenshot 2022-10-18 at 17 09 08

@JeroenReumkens
Copy link
Copy Markdown
Contributor Author

@CarinaWolli Thanks for testing!! Very weird, I will check it! 🙏

@leog
Copy link
Copy Markdown
Contributor

leog commented Oct 18, 2022

@JeroenReumkens I was testing it in https://cal-auu1mnfp0-cal.vercel.app/bookings/upcoming and clicking in the popover was actually sending me to the booking success page 🤔

@JeroenReumkens
Copy link
Copy Markdown
Contributor Author

@leog Thanks, will also retest that tomorrow! In theory this line should prevent that, but then it doesn't seem to do that according to your tests 🤔 Will recheck.

@zomars
Copy link
Copy Markdown
Contributor

zomars commented Oct 18, 2022

@JeroenReumkens let's also do a self-review from now.

const location = booking.location || "";

const onClick = () => {
const onClickTableData = (ev: React.MouseEvent<HTMLDivElement>) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@JeroenReumkens Renamed to better understanding and don't collide with onClick.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@alannnc Cool! Main reason I added it in here instead of the default behavior of the Popover is because the click behavior is actually something the BookingListItem tries to control, and not something the Popover should know about. But I sticked with your suggestion and added it to the tooltip button as well. Abstracted it to a function to so I could add a comment as to why this is needed 👍

Comment thread packages/ui/v2/core/MeetingTimeInTimezones.tsx Outdated
Comment thread packages/lib/date-fns/index.ts
Copy link
Copy Markdown
Contributor

@alannnc alannnc left a comment

Choose a reason for hiding this comment

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

Code works now.
NIT: What do you think of adding this so you know which one is yours.

image

@alannnc alannnc added the ♻️ autoupdate tells kodiak to keep this branch up-to-date label Oct 18, 2022
Copy link
Copy Markdown
Contributor

@zomars zomars left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: alannnc <alannnc@gmail.com>
… compared agains a date instead of time. Also improved sortbytimezone util by making use of utcoffset helper from dayjs.
@JeroenReumkens
Copy link
Copy Markdown
Contributor Author

Fixes mentioned in PR are done. Also make the sortByTimezone function simpler by utilising the utcOffset in dayjs, which I did not discover during my search for something like this before 😅

@CarinaWolli There indeed was a bug in comparing prev / next day 🙏 Thanks so much for discovering this. This is fixed now. Could you please retest it?

@leog Clicking the popup content now doesn't redirect you to the detail page anymore 😄 Thanks for spotting that, I actually only tested clicking the button, not the content I realised! Could you please retest?

@alannnc I reverted your date format change for #5050 as you told me to. I did NOT revert all the other changes in that PR, was that actually the plan?

Thanks all!

@PeerRich PeerRich enabled auto-merge (squash) October 19, 2022 09:45
@PeerRich PeerRich merged commit 373c255 into main Oct 19, 2022
@PeerRich PeerRich deleted the feat/3334-show-times-in-timezones-for-bookingpage branch October 19, 2022 09:45
haffla pushed a commit to tourlane/cal.com that referenced this pull request Nov 22, 2022
Co-authored-by: alannnc <alannnc@gmail.com>
Co-authored-by: Alan <alannnc@gmail.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
haffla pushed a commit to tourlane/cal.com that referenced this pull request Nov 22, 2022
Co-authored-by: alannnc <alannnc@gmail.com>
Co-authored-by: Alan <alannnc@gmail.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

♻️ autoupdate tells kodiak to keep this branch up-to-date

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CAL-74] Always list Time Zones

6 participants