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

🔧 libs/ui build error #8267

Merged
merged 4 commits into from
Nov 27, 2023
Merged

🔧 libs/ui build error #8267

merged 4 commits into from
Nov 27, 2023

Conversation

roiLeo
Copy link
Contributor

@roiLeo roiLeo commented Nov 27, 2023

-related with #8251

Who made TheImage component? name is misleading & component import is bad.

Capture d’écran 2023-11-27 à 10 55 23 AM

@roiLeo roiLeo requested a review from a team as a code owner November 27, 2023 09:57
@roiLeo roiLeo requested review from preschian and daiagi and removed request for a team November 27, 2023 09:57
Copy link

netlify bot commented Nov 27, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit d31503f
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/6564b2d9d3f6d10008f2f4b8
😎 Deploy Preview https://deploy-preview-8267--koda-canary.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.

@reviewpad reviewpad bot added small Pull request is small waiting-for-review labels Nov 27, 2023
Copy link
Contributor

reviewpad bot commented Nov 27, 2023

AI-Generated Summary: This pull request contains code modifications for primarily three files: ImageMedia.vue, NeoAvatar.vue, and TheImage.vue located in libs/ui/src/components. The changes mainly revolve around importing additional utilities from 'vue' such as 'defineProps', 'withDefaults', and 'computed' into these files. This could be a step towards enhancing the functional components within the Vue ecosystem of this project. Also, in ImageMedia.vue, 'consola' has been imported which is generally used for debugging, suggesting some debugging activity or enhanced logging could be expected in the future. Consequently, these changes are focused on improving the codebase usability and modularity.

@vikiival
Copy link
Member

Maybe rollback CODEOWNERS?

@roiLeo
Copy link
Contributor Author

roiLeo commented Nov 27, 2023

Maybe rollback CODEOWNERS?

why?

@preschian
Copy link
Member

Maybe rollback CODEOWNERS?

why?

maybe assign it to internal dev?

.github/CODEOWNERS Outdated Show resolved Hide resolved
Copy link

codeclimate bot commented Nov 27, 2023

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

View more on Code Climate.

@preschian
Copy link
Member

anyway, can we capture the error on ci with some commands?
maybe put pnpm -F brick story:build on ci

@roiLeo
Copy link
Contributor Author

roiLeo commented Nov 27, 2023

anyway, can we capture the error on ci with some commands? maybe put pnpm -F brick story:build on ci

https://github.com/kodadot/nft-gallery/blob/main/.github/workflows/build.libs-ui.yml
or #4464

@shashkovdanil
Copy link
Contributor

shashkovdanil commented Nov 27, 2023

It's my component. Yes, I noticed that the absence of imports breaks historie, is there any way to customize the linter to avoid such problems in the future? We don't need to do explicit imports in the main project, but in ui kit we do, because of this problem. These two projects are in one repository, which is differently configured. I would like to solve such a problem in a centralized way

@shashkovdanil
Copy link
Contributor

shashkovdanil commented Nov 27, 2023

@roiLeo btw this PR will not fix this issue. This issue is due to the fact that we use the $t function in our code in libs/ui, for example here {{ $t('lewd.explicit') }}, which causes the error

@roiLeo
Copy link
Contributor Author

roiLeo commented Nov 27, 2023

is there any way to customize the linter to avoid such problems in the future?

move project outside repo

We don't need to do explicit imports in the main project, but in ui kit we do, because of this problem.

can be fixed if we migrate to Vue histoire to Nuxt histoire

I would like to solve such a problem in a centralized way

Let us know how you would proceed before starting anything

@roiLeo
Copy link
Contributor Author

roiLeo commented Nov 27, 2023

$t('lewd.explicit')

same as #8124 (comment) we don't need i18n in UI package, This MediaItem should be in kodadot components instead

@shashkovdanil
Copy link
Contributor

Let us know how you would proceed before starting anything

I agree with you about separating repo with its own build process, also setting up linter there as well to avoid these problems

@shashkovdanil
Copy link
Contributor

This MediaItem should be in kodadot components instead

Well we have to have this in UI kit:
image

but MediaItem should be in main repo? Did I get that right?

@shashkovdanil
Copy link
Contributor

shashkovdanil commented Nov 27, 2023

About the naming of this component, this is a common Image component, I named it along the lines of TheButton. What are some other options for naming the common component of the image?

@roiLeo
Copy link
Contributor Author

roiLeo commented Nov 27, 2023

About the naming of this component, this is a common Image component, I named it along the lines of TheButton. What are some other options for naming the common component of the image?

I don't think it's a good idea to prefix with The*, if it's common then it should be Common* like CommonPrimaryButton, CommonInvertedButton etc.. we can use Nuxt auto import feature for better naming (/components/common/*)

@shashkovdanil
Copy link
Contributor

shashkovdanil commented Nov 27, 2023

What about BaseImage? This is actually a component that should replace the default html tag . This component is not a UI component because it does not have any styles

@roiLeo
Copy link
Contributor Author

roiLeo commented Nov 27, 2023

What about BaseImage? This is actually a component that should replace the default html tag . This component is not a UI component because it does not have any styles

What will be the use case of this BaseImage component? one single component for all images?

@shashkovdanil
Copy link
Contributor

Use instead of the tag in the UI kit, i.e. it's just a wrapper over img into which you can additionally pass a component like NuxtImg

@roiLeo roiLeo mentioned this pull request Nov 27, 2023
@roiLeo roiLeo linked an issue Nov 27, 2023 that may be closed by this pull request
2 tasks
Copy link
Contributor

reviewpad bot commented Nov 27, 2023

Reviewpad Report

⚠️ Warnings

  • Please link an issue to the pull request

@roiLeo roiLeo removed a link to an issue Nov 27, 2023
2 tasks
@roiLeo roiLeo added this pull request to the merge queue Nov 27, 2023
Merged via the queue into kodadot:main with commit 2b34f40 Nov 27, 2023
12 of 15 checks passed
@roiLeo roiLeo deleted the fix/ui/build branch November 27, 2023 15:17
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.2% 0.2% Duplication

This was referenced Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
small Pull request is small waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants