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

feat(#488): Area ve location requestlerinin ayrilmasi #533

Merged
merged 22 commits into from
Feb 9, 2023

Conversation

cdagli
Copy link
Contributor

@cdagli cdagli commented Feb 9, 2023

Açıklama

Bu PR yaptigimiz tek requesti, 2 farkli request olacak sekilde refactor ediyor. areas-lite icerisinde sadece id, lat, lng aliyoruz.

Geri kalan detay verileri ise drawer acildiginda locations/{locationId} ile aliyoruz.

discord kullanıcı adı: @canerdagli

Closes #488 and possibly #521

Loading:

Error:
image
image

Lütfen değişikliklerinizi açıklayın. Aynı zamanda amacınızı ve içeriği açıklayın. Bu değişikliklerin gerektirdiği bağımlılıkları da listelemeyi unutmayın.

## PR açmadan önce dikkat edilmesi gerekenler

  • Kendi kodumu inceledim, review ettim.
  • Eğer core bir feature ise, detaylı testler yaptım.

PR açma kuralları

  • Approved tagine sahip bir issue için PR açılmalıdır. Aksi takdirde PR reddedilecektir.
  • İlgili issue numarası: PR ile ilgili issue numarası, PR başlığının başına, prefix sonrası parantez içinde # ile eklenmelidir. "prefix(#issue_number): PR başlığı" şeklinde bir başlık kullanılabilir.
  • 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(#issueId): Add README.md" şeklinde bir başlık kullanılabilir.
  • İ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.

Değişiklikler

  • Yeni bir özellik (breaking change olmayan, yeni bir özellik ekleyen bir değişiklik)
  • Yeni bir refactor (breaking change olmayan, kodun okunabilirliğini veya performansını artıran bir değişiklik)
  • Breaking bir değişiklik
  • Dokümantasyon değişikliği

Bu değişiklikler nasıl test edildi?

Lütfen yaptığınız değişiklikleri test etmek için yaptığınız testleri açıklayın. Lütfen aynı zamanda test konfigürasyonunuzu da belirtin.

Test Konfigürasyonu:

  • Firmware versiyonu:
  • Donanım:
  • Toolchain:
  • SDK:

@vercel
Copy link

vercel bot commented Feb 9, 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 9, 2023 at 1:57AM (UTC)

pt: "4px",
}}
>
{data?.baseMarker?.formatted_address}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bunu gosteremiyoruz cunku artik formatted_addressi areas-litedan almiyoruz

@cdagli cdagli changed the title [488] Request refactoring feat(#488): Area ve location requestlerinin ayrilmasi Feb 9, 2023
@cdagli cdagli marked this pull request as ready for review February 9, 2023 00:26
@cdagli
Copy link
Contributor Author

cdagli commented Feb 9, 2023

Biraz error handling ekliyorum..

@cdagli cdagli marked this pull request as draft February 9, 2023 00:29
@cdagli cdagli marked this pull request as ready for review February 9, 2023 00:42
@altankurt
Copy link

#521 "approved" değil. @cdagli

@cdagli
Copy link
Contributor Author

cdagli commented Feb 9, 2023

@altankurt #488 ve #521 ayni isler (ya da cok benzer)

@altankurt altankurt added the kurala-uygun-pr this PR is compatible with our PR regulations label Feb 9, 2023
<Button
variant="outlined"
className={styles.clipboard}
{isLoading && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aslinda SWR icin da global loading handler implement edilebiliyor ama ayri bir is: vercel/swr#942 (comment)

{error && (
<Box
sx={{
height: "300px",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Buraya fixed bir height vermeden cozmenin bir yolu var mi?

(Bunu vermezsek asagidan cilar drawer loading indicator heighti kadar oluyor)

Copy link
Contributor

Choose a reason for hiding this comment

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

Error component'ini component/Error altinda generic hale getirsek daha kullanisli olabilir.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed height yerine responsive min height ayarlamak daha esnek olur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SkeletonLoader koyabiliriz ama onlara da fixed bir height vermek gerekir.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aslinda bu error componenti cok genericmis. Butun return kismi suna donusse aslinda cok guzel olur bence:

if (error) return <Error />;
if (isLoading) return <Spinner />
if (!data) return <Empty />

return (...)

error componentini minHeight ile istedigimiz gibi kullanabiliriz.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@buraksaraloglu Katiliyorum, bence butun Drawer'in refactor edilmesi lazim. Baska bir PR'da yapsak olur mu?

}}
>
<Typography variant="h5">
Teknik bir sorun oluştu. Lütfen daha sonra tekrar deneyiniz.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generic bir sey ekledim, tavsiyelere acigim.

</Typography>
</Box>
)}
{!isLoading && data && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Buradan sonrasi ayni kod, herhangi bir sey eklemedim. Sadece useMemonun dependency arrayine isLoading ve error ekledim.

@@ -1,3 +1,3 @@
export const BASE_URL = "https://apigo.afetharita.com/feeds/areas";
export const BASE_URL = "https://api.afetharita.com";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: backendden haber geldiginde URL'i degistirebiliriz.

{error && (
<Box
sx={{
height: "300px",
Copy link
Contributor

Choose a reason for hiding this comment

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

Error component'ini component/Error altinda generic hale getirsek daha kullanisli olabilir.

{error && (
<Box
sx={{
height: "300px",
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed height yerine responsive min height ayarlamak daha esnek olur.

);

useEffect(() => {
if (isOpen) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Butun fonksiyonu if ile sarmalamak yerine useEffect'in basinda if (!isOpen) return; diyerek okunabilirligi arttirabilirsin.

<Typography>{raw?.full_text}</Typography>
</div>
)}
{!showSavedData && (
Copy link
Contributor

Choose a reason for hiding this comment

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

266 satir ile bu satiri neden ayri ayri kontrol ediyoruz? ? : yeterli olmasi lazim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bilmiyorum ama fix ettim 👍

@cdagli cdagli merged commit ecdb052 into acikyazilimagi:development Feb 9, 2023
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.

feat: Marker'a tıklandığı zaman detay bilgilerinin yeni endpoint üzerinden id ile alınması
6 participants