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: move media from rmrk/Media/ to media/ #3718

Merged
merged 30 commits into from
Aug 16, 2022

Conversation

preschian
Copy link
Member

@preschian preschian commented Aug 12, 2022

Thank you for your contribution to the KodaDot NFT gallery.
👇 _ Let's make a quick check before the contribution.

PR type

  • Bugfix
  • Feature
  • Refactoring

What's new?

Before submitting Pull Request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged the recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Link Preview

Type Link
Image https://deploy-preview-3718--koda-nuxt.netlify.app/rmrk/gallery/13438802-3892e43e923e5ad973-KAG-MINE_17-0000000000000457
3D https://deploy-preview-3718--koda-nuxt.netlify.app/rmrk/gallery/11380735-629a449743cd29bf21-3DGINO-YGGDRASIL-0000000000000041
Audio https://deploy-preview-3718--koda-nuxt.netlify.app/rmrk/gallery/11996182-ee8d75dede329d1224-SKMP-18_WHOLE_EPISODE_WITH_JARED_NATIONS-0000000000000058
Video https://deploy-preview-3718--koda-nuxt.netlify.app/rmrk/gallery/12049607-4466b2edfa4d261069-POLKAWEAR-YOROI_GIFT_CARD_15-0000000000000023
Landing page https://deploy-preview-3718--koda-nuxt.netlify.app/
Collections https://deploy-preview-3718--koda-nuxt.netlify.app/rmrk/explore
Gallery https://deploy-preview-3718--koda-nuxt.netlify.app/rmrk/explore?tab=GALLERY&page=1

Community participation

@netlify
Copy link

netlify bot commented Aug 12, 2022

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit 4de15a3
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/62fb9b16578c18000977d4cb
😎 Deploy Preview https://deploy-preview-3718--koda-nuxt.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 settings.

@preschian preschian mentioned this pull request Aug 12, 2022
18 tasks
@preschian preschian marked this pull request as ready for review August 12, 2022 07:48
@preschian preschian requested review from a team as code owners August 12, 2022 07:48
@preschian preschian requested review from petersopko and removed request for a team August 12, 2022 07:48
@petersopko petersopko requested a review from vikiival August 12, 2022 08:10
@petersopko
Copy link
Contributor

@vikiival can I get your pair of 👀 at this?

@vikiival
Copy link
Member

Pls maintain the order of the SFC

https://vuejs.org/api/sfc-spec.html#overview

@vikiival
Copy link
Member

Isn't it better to use

import { defineProps } from 'vue'

than

import { defineProps } from '#app'

asking tbh

Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

Hey please maintain the order of the SFC - otherwise it's really hard to do the Code Review

components/media/AudioMedia.vue Outdated Show resolved Hide resolved
components/media/ImageMedia.vue Outdated Show resolved Hide resolved
@preschian
Copy link
Member Author

Hey please maintain the order of the SFC - otherwise it's really hard to do the Code Review

oops sorry, will update

Isn't it better to use
import { defineProps } from 'vue'

there are some nuxt-specific, for example, useNuxtApp
but, when I peek at their ./.nuxt/tsconfig.json, they have another alternative. which one do you prefer?

import { useNuxtApp } from '#app'
import { useNuxtApp } from 'nuxt3/app'
import { useNuxtApp } from 'nuxt/app'

btw, I forgot to remove defineProps. no need to import https://vuejs.org/api/sfc-script-setup.html#defineprops-defineemits

@preschian
Copy link
Member Author

Nice update, now auto imports don't throw errors anymore.

Last thing, It seems that I can't run linter anymore: pnpm run lint

throw:

Oops! Something went wrong! :(

ESLint: 8.21.0

ESLint couldn't find the config "prettier" to extend from. Please check that the name of the config is correct.

The config "prettier" was referenced from the config file in "...../nft-gallery/.eslintrc.js".

If you still have problems, please stop by https://eslint.org/chat/help[](https://eslint.org/chat/help) to chat with the team.

works well when I remove 'prettier' from extends in .eslintrc config file

oh, try to run pnpm install first

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

oh, try to run pnpm install first

^ this, logical knowing that CI had no error

@roiLeo roiLeo requested a review from vikiival August 15, 2022 10:51
package.json Show resolved Hide resolved
Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

Smol stuff.

Also I do not understand why you deleted old components :)
For the next time:

  • mv to new location
  • rename (if needed)
  • refactor

This way we lost a lot of info 🥲

@preschian
Copy link
Member Author

preschian commented Aug 15, 2022

Smol stuff.

Also I do not understand why you deleted old components :) For the next time:

  • mv to new location
  • rename (if needed)
  • refactor

This way we lost a lot of info 🥲

oops, tbh I'm curious about that also. afaik I moved the files, but let me try to re-moved that files again from the suggested steps 🙏🏻

update:
when I moved the files, GitHub was able to detect moved files as File renamed without changes.. but, when I changed it to use composition API in a different commit, git was not able to detect moved files. seems like GitHub has a kind of threshold diff https://stackoverflow.com/a/433142

I tried with git mv not work also. next, if I get this again better to move the files only. open different PR for refactor to composition API

@preschian preschian requested a review from vikiival August 16, 2022 06:20
@yangwao
Copy link
Member

yangwao commented Aug 16, 2022

Added nuxt/framework#3751 to desc as it's fixing stuff 😄

@codeclimate
Copy link

codeclimate bot commented Aug 16, 2022

Code Climate has analyzed commit 4de15a3 and detected 0 issues on this pull request.

View more on Code Climate.

@yangwao
Copy link
Member

yangwao commented Aug 16, 2022

Tests are failing 😰

@preschian
Copy link
Member Author

preschian commented Aug 16, 2022

Tests are failing 😰

let me retry the pipeline again, I got the same error also previously. it works after retrying

@preschian
Copy link
Member Author

Tests are failing 😰

this is the details regarding the test "sometimes" failing #3755

@yangwao
Copy link
Member

yangwao commented Aug 16, 2022

okay let's try

@yangwao
Copy link
Member

yangwao commented Aug 16, 2022

seems everything builds well

image

@yangwao yangwao merged commit 8305d56 into kodadot:main Aug 16, 2022
@cazarov cazarov mentioned this pull request Aug 16, 2022
20 tasks
@roiLeo roiLeo mentioned this pull request Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Refactor: landing page
5 participants