Closed ShrootBuck closed 1 year ago
The latest updates on your projects. Learn more about Vercel for Git ↗︎
Name | Status | Preview | Comments | Updated (UTC) |
---|---|---|---|---|
open-resume | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Jul 4, 2023 4:47am |
Made this in like 5 minutes, so I apologize if there's any issues as I've not familiarized myself with the codebase.
Thanks so much for creating this PR!! You got it and found the right place. It looks great, I just left a nit.
Taking a step back, do you mind explaining what is your use case for adding tel
and why is it desirable?
Similarly to the mailto, it's just a bit easier to get around. Like when I'm on a mac, I can just click it and instantly call the number (and I would bet that Windows and Linux have similar functionality). Now, I'm not a recruiter, but I imagine most recruiters are looking at resumes from PDFs, not printed copies. It's more of a convenience use case, but if you don't agree we can close PR.
Thanks for the clarification. I don't see any downside on adding it so let's add it if it could be helpful. I am a windows user and wasn't aware that Mac can call the number. Does it trigger the phone to make a call or something? I will test it on windows too. I was imagining more of a mobile phone use case, and that with mobile phone getting more accessible, it might be helpful to click the number on the pdf to initiate a call.
Oh well if you're viewing on mobile, then yeah it can call directly. I used Mac as my use case because of how amazingly well Apple ties it to the iPhone. I just checked, if you connect your phone to windows using Phone Link, it can create calls with the tel link as well.
Also, PS I added another commit just for a quick... bug fix? I'm not sure it's considered a bug but definitely could open up unintended behavior.
Thanks for testing it on Mac, glad to know it works great! I also just test it on my phone and it works great too. Nice enhancement indeed.
Good catch on the potential url bug! My initial intent is to hope users don't add their urls with http(s) prefix and the placeholder example doesn't include the prefix. But things can happen and this is a good safeguard.
The PR looks great. I have some personal nit preference comments and just went ahead to push some changes given the changes are small, e.g.
http_protocol
(python convention) to camel case httpProtocol
(JS/TS convention)value.substring(item.length - 1, value.length - 1)
is equivalent of the shorter value.substring(item.length - 1)
{}
in each switch case block to make things slightly more clear, e.g. similar to if blockssrc = value.startsWith("http") ? value :
https://${value};
, so just made a change thereLet me know what you think of the new change and if it is ready to merge? Thanks again!
Sorry about all that stuff, I'm still fairly new to JS. Looks ready to me though!
No worries. Thanks again for creating this great PR. Hope you are liking JS. Just merged.
Similar to "mailto", adds the "tel" link support