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: setting to render NFTs image #5816

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

lukicenturi
Copy link
Contributor

@lukicenturi lukicenturi commented Mar 27, 2023

Closes #3975

image
image

@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #5816 (b93af3f) into develop (2c7a0af) will increase coverage by 0.08%.
The diff coverage is 87.55%.

@@             Coverage Diff             @@
##           develop    #5816      +/-   ##
===========================================
+ Coverage    72.69%   72.78%   +0.08%     
===========================================
  Files         1078     1083       +5     
  Lines       111564   111789     +225     
  Branches      9834     9873      +39     
===========================================
+ Hits         81101    81361     +260     
+ Misses       28997    28963      -34     
+ Partials      1466     1465       -1     
Flag Coverage Δ
frontend_integration 60.44% <29.29%> (-0.29%) ⬇️
frontend_unit 77.67% <95.78%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nts/accounts/management/inputs/InputModeSelect.vue 76.92% <0.00%> (+18.09%) ⬆️
...d/app/src/components/settings/FrontendSettings.vue 100.00% <ø> (ø)
...src/components/settings/general/RefreshSetting.vue 100.00% <ø> (ø)
...nts/settings/general/nft/NftsInNetValueSetting.vue 100.00% <ø> (ø)
frontend/app/src/pages/settings/general/index.vue 100.00% <ø> (ø)
...otkehlchen/chain/ethereum/modules/curve/decoder.py 25.35% <ø> (+0.05%) ⬆️
.../settings/general/nft/NftImageRenderingSetting.vue 50.00% <50.00%> (ø)
frontend/app/src/composables/assets/nft.ts 89.28% <66.66%> (ø)
frontend/app/src/utils/url.ts 93.54% <93.54%> (ø)
frontend/app/src/components/helper/NftDetails.vue 100.00% <100.00%> (ø)
... and 7 more

... and 12 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lukicenturi lukicenturi force-pushed the 3975-setting-to-render-nft-image branch from 87690e2 to 5db0359 Compare March 27, 2023 08:03
@lukicenturi lukicenturi force-pushed the 3975-setting-to-render-nft-image branch 16 times, most recently from 48f0d67 to 159a0f6 Compare March 30, 2023 18:30
@kelsos
Copy link
Member

kelsos commented Apr 3, 2023

@lukicenturi what is remaining for this? is it ready to re-check?

@lukicenturi
Copy link
Contributor Author

@lukicenturi what is remaining for this? is it ready to re-check?

Yes it is

frontend/app/src/data/psl.ts Outdated Show resolved Hide resolved
@kelsos
Copy link
Member

kelsos commented Apr 3, 2023

Another question is do you think it would be possible to make it so that you can whitelist a domain from the image component in the nonfungible balances?

@lukicenturi lukicenturi force-pushed the 3975-setting-to-render-nft-image branch from 159a0f6 to 96f3ba5 Compare April 4, 2023 03:49
@lukicenturi
Copy link
Contributor Author

Another question is do you think it would be possible to make it so that you can whitelist a domain from the image component in the nonfungible balances?

Yeah it can be worked on

@lukicenturi lukicenturi force-pushed the 3975-setting-to-render-nft-image branch 12 times, most recently from bad546b to dcdba80 Compare April 5, 2023 08:10
@lukicenturi lukicenturi force-pushed the 3975-setting-to-render-nft-image branch from dcdba80 to b9ff1fa Compare April 5, 2023 08:13
Copy link
Member

@kelsos kelsos left a comment

Choose a reason for hiding this comment

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

Ok, so I left a small comment, but overall it is good.

Also, I am thinking, if it would make sense for the table to either show the domain as part of the tooltip or even make it so that you can copy it, what do you think?

For the gallery, you have the whitelist button which makes things easier, but for the table, while you have access to the settings you still don't know the domain.

frontend/app/src/components/nft/NftGalleryItem.vue Outdated Show resolved Hide resolved
@lukicenturi
Copy link
Contributor Author

Ok, so I left a small comment, but overall it is good.

Also, I am thinking, if it would make sense for the table to either show the domain as part of the tooltip or even make it so that you can copy it, what do you think?

For the gallery, you have the whitelist button which makes things easier, but for the table, while you have access to the settings you still don't know the domain.

So for the table, I make the image clickable, to add it to the whitelisted domains

@lukicenturi lukicenturi force-pushed the 3975-setting-to-render-nft-image branch from b9ff1fa to b93af3f Compare April 5, 2023 13:37
@kelsos
Copy link
Member

kelsos commented Apr 5, 2023

Ok, so I left a small comment, but overall it is good.
Also, I am thinking, if it would make sense for the table to either show the domain as part of the tooltip or even make it so that you can copy it, what do you think?
For the gallery, you have the whitelist button which makes things easier, but for the table, while you have access to the settings you still don't know the domain.

So for the table, I make the image clickable, to add it to the whitelisted domains

As long as you have to confirm first that should be fine :)

@lukicenturi
Copy link
Contributor Author

Ok, so I left a small comment, but overall it is good.
Also, I am thinking, if it would make sense for the table to either show the domain as part of the tooltip or even make it so that you can copy it, what do you think?
For the gallery, you have the whitelist button which makes things easier, but for the table, while you have access to the settings you still don't know the domain.

So for the table, I make the image clickable, to add it to the whitelisted domains

As long as you have to confirm first that should be fine :)

It's updated now

Copy link
Member

@kelsos kelsos left a comment

Choose a reason for hiding this comment

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

lgtm @lukicenturi thank you very much

@kelsos kelsos merged commit 6597c0e into rotki:develop Apr 5, 2023
12 checks passed
@kelsos kelsos deleted the 3975-setting-to-render-nft-image branch April 5, 2023 17:28
@kelsos kelsos temporarily deployed to cassette-merge April 5, 2023 17:28 — with GitHub Actions Inactive
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.

Rendering images hosted by a malicious 3rd party can lead to ip leaks
2 participants