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.

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

Altair59
Copy link

@Altair59 Altair59 commented Mar 7, 2024

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

  • [Done] location input option interaction
  • [Done] location edit modal layout & interaction(adding/editing/removing)
  • [Done] location display in Tweet input form
  • [TODO] Inject world cities(format {city: STRING, country: STRING}) into Firestore (current location options are hard coded)
  • [Done] Add location data field to Tweet
  • [Done] Integrate location into Tweet when creating/displaying Tweet in view
  • [Done] Add mobile end display support

Copy link

vercel bot commented Mar 7, 2024

Someone is attempting to deploy a commit to a Personal Account owned by @ccrsxx on Vercel.

@ccrsxx first needs to authorize it.

@Altair59 Altair59 marked this pull request as draft March 7, 2024 09:29
@Ketchupchh
Copy link
Contributor

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.

@Altair59
Copy link
Author

Altair59 commented Mar 8, 2024

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:

  1. Create cities Document on Cloud Firestore (import from local static cities data CSV/JSON), but this needs to be done by the maintainer as I don't have access to the DB. Afterwards, I can implement the service and APIs to fetch the cities data in the component.
  2. Use geo location web service like Geonames, to query the cities data every time. I don't like this one as most free geo APIs have query limits.

What are your thoughts? @ccrsxx @Ketchupchh

@Ketchupchh
Copy link
Contributor

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.
image

@Altair59
Copy link
Author

Altair59 commented Mar 8, 2024

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. image

Handling overflow on my end and have a rough fix now. But looks like TweetDate rendered twice on mobile end is a problem on the Master deployment here... I'll try to look for a fix for this first and include my overflow fix, or double dates won't leave any space for location....

@Ketchupchh
Copy link
Contributor

Ketchupchh commented Mar 8, 2024

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.

@Altair59
Copy link
Author

Altair59 commented Mar 8, 2024

Yes, I'm aware of this. I don't think it 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.

@Altair59
Copy link
Author

Altair59 commented Mar 9, 2024

Fixed the Tweet Date double rendering issue and added my overflow fix on mobile end. Please check them if have time. :)

@Ketchupchh
Copy link
Contributor

Everything looks fine on my end :). Last thing is to support all locations. Have you checked out the Firebase Geo queries yet?

@Altair59
Copy link
Author

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.

@ccrsxx
Copy link
Owner

ccrsxx commented Mar 12, 2024

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.

Copy link

vercel bot commented Mar 31, 2024

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

Name Status Preview Comments Updated (UTC)
twitter-clone ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 9, 2024 10:24am

@ccrsxx
Copy link
Owner

ccrsxx commented Mar 31, 2024

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?

@Altair59
Copy link
Author

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 city and country info, I have stored the locations locally in a JSON file which is not too large.

Does this sound OK to you? @ccrsxx

@Altair59 Altair59 marked this pull request as ready for review April 29, 2024 04:55
@ccrsxx
Copy link
Owner

ccrsxx commented May 9, 2024

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.

@Ketchupchh
Copy link
Contributor

@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.

image

h-lunah added a commit to h-lunah/OpenTwitter that referenced this pull request Dec 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants