-
Notifications
You must be signed in to change notification settings - Fork 183
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.
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
[WIP] Added location for Tweet input #48
base: main
Are you sure you want to change the base?
Conversation
Someone is attempting to deploy a commit to a Personal Account owned by @ccrsxx on Vercel. @ccrsxx first needs to authorize it. |
I'm sure all contributions are welcomed :). Mention me once you have everything implemented. You also haven't implemented it for mobile yet either from what I can see. |
The front-end component and interactions are almost done. Feel free to test it here. However, currently for location select I'm still using a hard-coded {city: "city", country: "country"} list for users to select from. I need some suggestions on how we should store the data. There are few options:
What are your thoughts? @ccrsxx @Ketchupchh |
You can probably just use Firebase for this. But there's probably a built in way to do this though I'm not sure lol :p Also make sure you manage the overflow so we don't get things like this especially on mobile. |
Handling overflow on my end and have a rough fix now. But looks like |
Yes, I'm aware of this. I don't think it's necessarily a problem with the code but more so that the data changes depending on what device you're using but I could totally be wrong. Regardless, you should always wrap your overflow in case more things need to be added to that space in the future. |
Sounds good! I'll see if I can find a quick fix, otherwise I'll just wrap any overflow in place. |
Fixed the Tweet Date double rendering issue and added my overflow fix on mobile end. Please check them if have time. :) |
Everything looks fine on my end :). Last thing is to support all locations. Have you checked out the Firebase Geo queries yet? |
Yeah I'm looking into it. BTW, thanks for reviewing! I'll ping you once everything's done. |
Hi thanks for the PR, currently I'm pretty busy with work and uni. I can take a look at the PR next week on the weekend. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Sorry, I'm just now looking at the PR, been pretty busy for the past three weeks. So currently, the data for the location is stored locally right? For the location I think you can use Place API from Google Maps. You can get the location by address or city with this. Or maybe just store the location locally if it just contains a city or a county, it should be pretty small. I went to check the location feature on the real Twitter or X (what it's called now) to see what it looks like, but the option is greyed out, can't click the location. Hmm why is it disabled or are there some settings I need to enable to use this lol? |
Hi there, sorry for the late response, I was busy with my intern the past 2 months. For the location feature on the real Twitter, you can enable it in https://twitter.com/settings/location. It's actually weird that it allows you to add any location tags to your posts irrelevant with your real location. So I have also implemented the location tag in a way that user can select any location tags. And yes, since the location tag feature only needs Does this sound OK to you? @ccrsxx |
No problem, I was busy too, and still am right now. I have an uni exam in the coming weeks and a bunch of final projects next week after that. So I can review it maybe after the next two or three weeks later. I took a quick look at your code, it looks ok. I'll review it in full by the time I'm free later. |
@Altair59 Searching locations in the Location Modal hangs and crashes on mobile (at least for me. Also haven't checked on pc). Not sure if this is related to this PR but when clicking and viewing a tweet, you get an application error now. |
Describe your changes
Hi there! Firstly thanks for open-sourcing this wonderful project. The whole app is cool and tidy.
So when exploring the app, I noticed that some of the Tweet input options were not implemented, so I was trying to implement the
location
feature.The deployment is available at here.
Issue ticket number and link
#49
Checklist
location
input option interactionlocation
edit modal layout & interaction(adding/editing/removing)location
display inTweet
input formlocation
data field toTweet
location
intoTweet
when creating/displayingTweet
in view