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

refactor: code from profile avatar image reduced motion setting issue #3069

Conversation

AndrewCrescencio
Copy link
Contributor

@AndrewCrescencio AndrewCrescencio commented Nov 29, 2024

Hi, just refactoring the code to fix the issue: #3044
I removed the props destructuring, because in the test log there is a warning about this, I also added the fallback to the avatar image as @shuuji3 told here #3066 (comment)

Copy link

netlify bot commented Nov 29, 2024

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit a75adee
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/6749134fa54a3f00082be5e5

Copy link

netlify bot commented Nov 29, 2024

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit a75adee
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/6749134fce500e0008aa935d
😎 Deploy Preview https://deploy-preview-3069--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

…scencio/fix/profile-avatar-image-reduced-motion-setting
@shuuji3 shuuji3 assigned shuuji3 and unassigned shuuji3 Nov 29, 2024
@shuuji3 shuuji3 self-requested a review November 29, 2024 10:20
@@ -1,7 +1,7 @@
<script setup lang="ts">
import type { mastodon } from 'masto'

const { account, square } = defineProps<{
const props = defineProps<{
Copy link
Member

Choose a reason for hiding this comment

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

@AndrewCrescencio Thanks for the fix!

About the object destructuring, that warning was valid in Vue 3.4 (the previous result of pnpm run test:ci), but Vue 3.5+ allows us to use the same syntax again. So we can still use the original destructuring on props. Could you revert that destructuring since we already used it in many other places if you don't mind? You can also check by running pnpm run test:ci on your environment.

These links will help to understand the latest (Vue 3.5+) usage of props destructuring:

I've updated the test result with the latest commit, that doesn't contains the previous warning: #2934

Copy link
Contributor Author

@AndrewCrescencio AndrewCrescencio Dec 1, 2024

Choose a reason for hiding this comment

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

Nice that we still can use props destructure, thanks for info, but let's leave it as it is, next time I know, I'll try to help with other things

Copy link
Member

@shuuji3 shuuji3 left a comment

Choose a reason for hiding this comment

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

By the way, that refactoring is optional. I'm happy to merge this and refactor it later with other similar usages.

@shuuji3 shuuji3 added this pull request to the merge queue Dec 1, 2024
Merged via the queue into elk-zone:main with commit b4cb027 Dec 1, 2024
14 checks passed
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.

2 participants