-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
➖ remove b-image component #5854
Conversation
Reviewpad Report
|
✅ Deploy Preview for koda-canary ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
AI-Generated Summary: This pull request includes updates to several Vue components, mainly focusing on replacing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meh.. I don't find any advantage to push this code in
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
everything pinted out by @Jarsen136
isn't it already the case? check
I don't understand your concern, do you want me to introduce new feature? this isn't the scope of this PR, only migration of our current component. IMO current fallback work as excepted.
Well component is named |
Yes, it's already there on the BasicImage component. I mean we could replace
I'm not about to introduce some new feature. I mention this because we remove b-image in this PR, and it would also remove some useful functionality provided by b-image.
The BasicImage naming is also ok to me. Could we replace |
Yes! Done for the one that was using
For now I don't think it's a good idea to replace all |
Yes, we do not need to replace all img tag.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small stuff. otherwise lgtm
Co-authored-by: Jarsen <31397967+Jarsen136@users.noreply.github.com>
@roiLeo |
looks good on my side |
Idk what happened but I lost some code from https://github.com/kodadot/nft-gallery/pull/5854/commits |
Code Climate has analyzed commit 9d38e10 and detected 0 issues on this pull request. View more on Code Climate. |
Merge? |
@prury can I ask you for one more check pls? Also @Jarsen136 && @preschian for review |
I would double check everywhere and slow roll with this update as we are using it a lot places and seen there was various bugs :) |
will do on my afternoon today |
Tested on Firefox/Chrome for Desktop |
this problem is still happening tho |
PR Type
Context
Before submitting pull request, please make sure:
Screenshot 📸
Copilot Summary
🤖 Generated by Copilot at 6f1458f
This pull request refactors several components that use images to improve performance, TypeScript support, and code quality. It replaces the
b-image
component from Buefy with nativeimg
elements, and applies some Vue style guide and Buefy conventions to the code. It also removes an empty line from thegraphqlResponseTypes.ts
file.🤖 Generated by Copilot at 6f1458f