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: resize images & fix Carousel (拆掉 commit 再重新 push) #25

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bessyhuang
Copy link
Contributor

No description provided.

Copy link
Contributor

@rileychh rileychh left a comment

Choose a reason for hiding this comment

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

看起來很棒,只有一些小問題。能讓滑鼠在 Swiper 上有「左、右」箭頭能點擊嗎?這樣會比較容易瀏覽相片。

另外,在手機上相片好像太小張了!試試看能不能讓元件的大小根據視窗的寬度縮放(或者在寬度不足時減少一次瀏覽的張數)。

Comment on lines 27 to 41
<swiper-container
:autoplay="{ delay: 2500, disableOnInteraction: false }"
:loop="true"
:pagination="{ clickable: true }"
:slides-per-view="3"
:space-between="10"
:speed="500"
>
<SwiperSlide
v-for="src in images"
:key="src"
>
<img :src="src">
</SwiperSlide>
</swiper-container>
Copy link
Contributor

Choose a reason for hiding this comment

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

diff --git a/components/FlickrCarousel.vue b/components/FlickrCarousel.vue
index 147cb53..7db7932 100644
--- a/components/FlickrCarousel.vue
+++ b/components/FlickrCarousel.vue
@@ -1,5 +1,5 @@
 <script setup>
-import { SwiperSlide } from 'swiper/vue'
+import { Swiper, SwiperSlide } from 'swiper/vue'
 import 'swiper/css'
 import 'swiper/swiper-bundle.css'
 import { register } from 'swiper/element/bundle'
@@ -24,7 +24,7 @@ const images = [image1, image2, image3, image4, image5, image6, image7, image8,
 </script>
 
 <template>
-  <swiper-container
+  <Swiper
     :autoplay="{ delay: 2500, disableOnInteraction: false }"
     :loop="true"
     :pagination="{ clickable: true }"
@@ -38,7 +38,7 @@ const images = [image1, image2, image3, image4, image5, image6, image7, image8,
     >
       <img :src="src">
     </SwiperSlide>
-  </swiper-container>
+  </Swiper>
 </template>
 
 <style>

看起來 swiper-container 不是一個元件,所以我猜你指的是 Swiper。這樣改後就沒有錯誤了~

Copy link
Contributor

Choose a reason for hiding this comment

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

補充一下,這邊的 swiper-container 是套件提供的 web component,在 register 之後, 瀏覽器應該就會認識這個 tag 了,不過這個註冊是直接註冊到 web component,所以對 vue 來說是未知的,所以才會有 vue 的警告。

https://github.com/nolimits4web/swiper/blob/40a705e5bcadf2ee2ee90591ff9ed95c1aaf9026/src/swiper-element.mjs#L309

Copy link
Contributor

Choose a reason for hiding this comment

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

謝謝你的指正!剛剛讀了一下 Swiper 的文件,好像 Swiper Vue 元件在未來會被棄用,並建議改用 web component。如果是這樣的話,是不是裡面的 Swiper 也要改用 web component,以求一致性呢?

Copy link
Contributor

@mirumodapon mirumodapon left a comment

Choose a reason for hiding this comment

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

LGTM, it is better if there is dynamic import at swiper image.

@bessyhuang
Copy link
Contributor Author

image
圖片顯示 待修

@rileychh
Copy link
Contributor

rileychh commented Oct 7, 2024

LGTM, it is better if there is dynamic import at swiper image.

為什麼動態引入會比較好呢?用靜態引入可以減少需要的 JavaScript 程式量。

import image9 from '#assets/images/image9.webp'
import image10 from '#assets/images/image10.webp'

register()
Copy link
Contributor

Choose a reason for hiding this comment

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

沒有要用 swiper/element (web components) 的話就不需要 import 和 register 了喔

v-for="src in images"
:key="src"
>
<img :src="src">
Copy link
Contributor

Choose a reason for hiding this comment

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

考慮為圖片加上 alt 屬性

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