Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Shows link location and respective icon in /bookings #11866

Merged
merged 27 commits into from
Oct 19, 2023
Merged

feat: Shows link location and respective icon in /bookings #11866

merged 27 commits into from
Oct 19, 2023

Conversation

Siddharth-2382
Copy link
Contributor

What does this PR do?

Shows link location and respective icon in /bookings.

Fixes #11730

/claim #11730

Screenshot 2023-10-12 at 11 26 06 PM

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • New feature (non-breaking change which adds functionality)

How should this be tested?

Once done a booking, go to the bookings page.

Mandatory Tasks

  • Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

@vercel
Copy link

vercel bot commented Oct 12, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 19, 2023 5:19am
ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 19, 2023 5:19am

@algora-pbc algora-pbc bot mentioned this pull request Oct 12, 2023
Closed
@vercel
Copy link

vercel bot commented Oct 12, 2023

@Siddharth-2382 is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added bookings area: bookings, availability, timezones, double booking Low priority Created by Linear-GitHub Sync ⚡ Quick Wins A collection of quick wins/quick fixes that are less than 30 minutes of work 💎 Bounty A bounty on Algora.io 🧹 Improvements Improvements to existing features. Mostly UX/UI labels Oct 12, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 12, 2023 •
edited
Loading

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link to collect XP and win prizes!

@Siddharth-2382 Siddharth-2382 changed the title Cal/meeting link feat: Shows link location and respective icon in /bookings Oct 12, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 12, 2023 •
edited
Loading

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

PeerRich
PeerRich previously requested changes Oct 13, 2023
View reviewed changes
Copy link
Member

@PeerRich PeerRich left a comment

Choose a reason for hiding this comment

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

we should not hard code logos and names, instead use whatever conference location was used

@PeerRich PeerRich added the ❗️invalid This doesn't seem right label Oct 13, 2023
@Siddharth-2382
Copy link
Contributor Author

Okay understood....will make the changes

@Siddharth-2382
Copy link
Contributor Author

View of bookings page after the latest changes:

Screen.Recording.2023-10-13.at.11.08.34.PM.mov

@PeerRich PeerRich mentioned this pull request Oct 17, 2023
Closed
2 tasks
sean-brydon
sean-brydon previously requested changes Oct 18, 2023
View reviewed changes
Copy link
Member

@sean-brydon sean-brydon left a comment

Choose a reason for hiding this comment

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

Overall this is great! Few nits that i think should be addressed

You can't click the link -> You have to middle click / right click open with tab (due to event propagation it seems)

Bookings with "link" as their location and also the likes of whereby just show empty with no location
CleanShot 2023-10-18 at 11 31 29@2x

@Siddharth-2382
Copy link
Contributor Author

Overall this is great! Few nits that i think should be addressed

You can't click the link -> You have to middle click / right click open with tab (due to event propagation it seems)

Bookings with "link" as their location and also the likes of whereby just show empty with no location CleanShot 2023-10-18 at 11 31 29@2x

Can you review again with the latest changes? I think I fixed that problem in the latest commits.

@sean-brydon
Copy link
Member

Overall this is great! Few nits that i think should be addressed
You can't click the link -> You have to middle click / right click open with tab (due to event propagation it seems)
Bookings with "link" as their location and also the likes of whereby just show empty with no location CleanShot 2023-10-18 at 11 31 29@2x

Can you review again with the latest changes? I think I fixed that problem in the latest commits.

Works perfect

@sean-brydon
Copy link
Member

Once type check is address im happy to merge this great work

@Siddharth-2382
Copy link
Contributor Author

Once type check is address im happy to merge this great work

done :)

@kremedev
Copy link
Contributor

kremedev commented Oct 18, 2023 •
edited
Loading

@PeerRich I got it! ;)

demo.mov

Why second booking link say "Join Meeting" when it redirects to Google Meet?

@Siddharth-2382
Copy link
Contributor Author

Siddharth-2382 commented Oct 19, 2023 •
edited
Loading

Because it's not a Google meet app meeting it's an external link meet which means it is supposed to show join meeting.

In cal you select meeting location like Google meet, zoom, cal video, in person, external meeting, etc.

@kremedev
Copy link
Contributor

FaceTime, WhatsApp, and Discord links are supposed to show "Join Meeting" too?

@Siddharth-2382
Copy link
Contributor Author

FaceTime, WhatsApp, and Discord links are supposed to show "Join Meeting" too?

If the meeting location is set to FaceTime, WhatsApp, Discord or any other app, it will show the respective app icon and say Join {APP NAME}. If the location of the meet is Link Meeting, it will just show the message Join Meeting. I hope you understood this.

@CarinaWolli CarinaWolli merged commit 2550485 into calcom:main Oct 19, 2023
@kremedev
Copy link
Contributor

It shows "Join Meeting" when meeting locations are FaceTime, WhatsApp, and Discord.

joinmeeting.mov

@Siddharth-2382
Copy link
Contributor Author

Those apps while having the meeting location of a particular app still needs to be given an external link and hence it shows Join Meeting as it is more of a general approach for any meetings that were given an external link and not just created by cal.

@Siddharth-2382 Siddharth-2382 deleted the cal/meeting-link branch October 21, 2023 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PeerRich PeerRich PeerRich approved these changes

@sean-brydon sean-brydon sean-brydon left review comments

Assignees
No one assigned
Labels
bookings area: bookings, availability, timezones, double booking 🙋 Bounty claim 💎 Bounty A bounty on Algora.io 🧹 Improvements Improvements to existing features. Mostly UX/UI ❗️invalid This doesn't seem right Low priority Created by Linear-GitHub Sync ⚡ Quick Wins A collection of quick wins/quick fixes that are less than 30 minutes of work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CAL-2590] Show link location in /bookings
5 participants
@Siddharth-2382 @sean-brydon @kremedev @PeerRich @CarinaWolli