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

fix: side menu - kaynak butonu twitter yönlendirme sorunu #482 #496

Closed
wants to merge 19 commits into from
Closed

Conversation

bb7hn
Copy link

@bb7hn bb7hn commented Feb 8, 2023

Created types for new extra_parameters field. parsed checked and redirected url. If cannot parse or values are invalid alerted a message

  • Approved tagine sahip bir issue için PR açılmalıdır. Aksi takdirde PR reddedilecektir.
  • Açıklayıcı ve anlaşılır bir başlık: PR başlığı, değişikliklerin niteliklerini ve amaçlarını açık bir şekilde tanımlamalıdır. Pr başlığı, PR açıldığında görüntülenen ilk şey olmalıdır. Ve semantik commit kurallarını takip etmelidir. Örneğin "docs: Add README.md" şeklinde bir başlık kullanılabilir.
  • İlgili issue numarası: PR ile ilgili issue numarası, PR başlığının sonuna # ile eklenmelidir.
  • İlgili dosya seçimi: Sadece ilgili dosyalara dokunulmalı ve başka dosyaların etkilenmemesi sağlanmalıdır.
  • Format ve Lint uygunluğu: Kod, belirli bir format standardına uygun hale getirilmeli ve lint kurallarına göre incelenmelidir.
  • Temiz commit geçmişi: Değişikliklerin yapıldığı commitler, anlaşılır ve düzenli olmalıdır.
  • İş tamamlandığında PR açılması: PR, iş tamamlandığında açılmalı ve diğer takım üyeleri tarafından incelenmesi için gönderilmelidir.

@vercel
Copy link

vercel bot commented Feb 8, 2023

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

Name Status Preview Comments Updated
deprem-yardim-frontend ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 8, 2023 at 10:43PM (UTC)

components/UI/Drawer/Drawer.tsx Outdated Show resolved Hide resolved
components/UI/Drawer/Drawer.tsx Outdated Show resolved Hide resolved
utils/constants.ts Outdated Show resolved Hide resolved
@bb7hn bb7hn requested a review from canumay February 8, 2023 18:56
@cdagli
Copy link
Contributor

cdagli commented Feb 8, 2023

tweet_id olmasina ragmen burasi yuklenmedi bende, sizde reproducible mi?

image

Su linkten denedim https://deprem-yardim-frontend-git-fork-bb7hn-development-afetharita.vercel.app/#lat=37.588306281721756&lng=36.892022135531754&zoom=16.75 ?

Error alert'i de goremedim, reproduce edebilecegim bir URL var mi?

if (extraParams.user_id && extraParams.tweet_id) {
router.push(
`https://twitter.com/${extraParams.user_id}/status/${extraParams.tweet_id}`
);
Copy link
Contributor

Choose a reason for hiding this comment

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

getTwitterLink tarzı bi util mi yazsak? google maps linkinde olduğu gibi

Copy link
Contributor

Choose a reason for hiding this comment

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

anyuser direkt kullanılabilinir bu arada burada. user id belirtilmeye gerek yok diye biliyorum. Önceki halide zaten bu şekilde çalışıyordu.

@bb7hn bb7hn requested review from cdagli and removed request for canumay February 8, 2023 20:27
@eraygundogmus eraygundogmus added the kurala-uygun-pr this PR is compatible with our PR regulations label Feb 8, 2023
@cdagli
Copy link
Contributor

cdagli commented Feb 8, 2023

@bb7hn Bunu fix ettik mi yoksa bilerek mi yaptik ?

@bb7hn
Copy link
Author

bb7hn commented Feb 8, 2023

@bb7hn Bunu fix ettik mi yoksa bilerek mi yaptik ?

@cdagli bu kısımda conditional rendering vardı RenderIf componentine çektim o kısımda condition hatalı kalmış şu an düzelttim

let extraParams: undefined | ExtraParameters;

try {
extraParams = JSON.parse(source.extra_parameters || "undefined");
Copy link
Contributor

Choose a reason for hiding this comment

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

Catch ettigimizde de undefined atayabilir miydik?

Copy link
Author

Choose a reason for hiding this comment

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

Kaynak linkine yönlendirme yaparken twitter iframe yüklenmesine rağmen link gitmiyordu.
Normalde endpointte channel gelirken açılmama sebebi channel gelmemesiymiş. o kısımda da düzenleme yapıp gönderim

Copy link
Contributor

@cdagli cdagli left a comment

Choose a reason for hiding this comment

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

@bb7hn Bir comment daha ekledim ama approve da ediyorum.

@cdagli
Copy link
Contributor

cdagli commented Feb 9, 2023

@bb7hn Maalesef dun #533 yuzunden bu PR conflict oldu. Ama hala burada isimize yarayacak fixler var. Rica etsem conflicti cozup, PR'i guncelleyebilir misin?

@cdagli
Copy link
Contributor

cdagli commented Feb 9, 2023

PR stale oldugu icin kapatiyorum bunu #589

@cdagli cdagli closed this Feb 9, 2023
@bb7hn
Copy link
Author

bb7hn commented Feb 9, 2023

PR stale oldugu icin kapatiyorum bunu #589

@cdagli Tamamdır yeni bakma şansım oldu sanırım gerekli değişiklikler yapıldı.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kurala-uygun-pr this PR is compatible with our PR regulations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants