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: add external link redirect #5369

Merged
merged 19 commits into from
Apr 3, 2023

Conversation

floyd-li
Copy link
Member

@floyd-li floyd-li commented Mar 25, 2023

Thank you for your contribution to the KodaDot NFT gallery.

this commit is to add the external link redirect page, i've added some hooks to prevent user open external link directly and will landing a redirect page to show some tips. also we can add whitelist and blacklist for the link so we can block some links for safety.

i think it's the first step for #5218 , just open this pr as draft and if anybody got better idea please leave comments here.

also, this one just a quick demo so forget the ui part :)

@yangwao @vikiival if the redirect logic is okay generally i'll continue the rest :)

👇 _ Let's make a quick check before the contribution.

PR Type

  • Bugfix
  • Feature
  • Refactoring

Context

Before submitting pull request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged 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

Optional

  • I've tested it at </bsx/collection>
  • I've tested PR on mobile
  • I've written unit tests 🧪
  • I've found edge cases

Had issue bounty label?

  • Fill up your KSM address: Payout

Community participation

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

image

image

@kodabot
Copy link
Collaborator

kodabot commented Mar 25, 2023

WARNING @floyd-li PR for issue #5218 which isn't assigned to you. Please be warned that this PR may get rejected if there's another assignee for issue #5218

@netlify
Copy link

netlify bot commented Mar 25, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit f920d19
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/642ae96ef38dc800088e833e
😎 Deploy Preview https://deploy-preview-5369--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 settings.

@exezbcz
Copy link
Member

exezbcz commented Mar 25, 2023

for which links are we going to use it? Only for links in nft description right. We can use modal that could look something like this
image

@floyd-li
Copy link
Member Author

floyd-li commented Mar 26, 2023

@exezbcz ahhhh seems i misunderstand it.
if it's only used in item description and just shows a tip before redirecting, modal is a better choice 😄

@yangwao
Copy link
Member

yangwao commented Mar 28, 2023

yes especially in links in descriptions, user profiles, and that's pretty much it.

I would put text
"You are going outside KodaDot to external website"

@yangwao
Copy link
Member

yangwao commented Mar 28, 2023

Test URL - https://deploy-preview-5369--koda-canary.netlify.app/rmrk/gallery/11946265-c210bd52d397099b2c-JPS-JAPANDAS_1231-0000000000001231

maybe could be done without visual redirect?

otherwise great @floyd-li ! :)

pages/redirect.vue Outdated Show resolved Hide resolved
@floyd-li
Copy link
Member Author

yes especially in links in descriptions, user profiles, and that's pretty much it.

I would put text "You are going outside KodaDot to external website"

got it. will continue this stuff after the notification box part.

little busy these days for dealing some personal stuff. so the task i taken may be delayed few days :(

@floyd-li
Copy link
Member Author

floyd-li commented Mar 31, 2023

updated: use some hack to resolve it - pass $i18n as props to RedirectModal, not smart but works XD

Modal.open({
  component: RedirectModal,
  canCancel: true,
  customClass: 'redirect-modal',
  props: {
    url,
    i18n: $i18n,
  },
} as unknown as BModalConfig)
<p>
  {{ props.i18n.t('redirect.safetyTips') }}
</p>

oops. i use $t to load translations and got error TypeError: Cannot read properties of undefined (reading '_t')
just use the simplest code, where goes wrong? 🤔️
if hard code text, it goes well. anyone got idea?

<p>
    {{ $t('redirect.safetyTips') }}
</p>

image

@floyd-li floyd-li marked this pull request as ready for review March 31, 2023 19:15
@floyd-li floyd-li requested a review from a team as a code owner March 31, 2023 19:15
@floyd-li floyd-li requested review from preschian and Jarsen136 and removed request for a team March 31, 2023 19:15
@floyd-li floyd-li changed the title [WIP, just for preview]feat: add external link redirect feat: add external link redirect Mar 31, 2023
Copy link
Member

@preschian preschian left a comment

Choose a reason for hiding this comment

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

fix deepsource, otherwise lgtm

I think it's better to wait for this PR #5366. so we can put useRedirect in one component

@vikiival
Copy link
Member

vikiival commented Apr 2, 2023

Hey, really nice job 🤗 .

Can I ask why you haven't used https://oruga.io/components/modal.html ?
We are rewriting a lot of ui with Oruga and Custom CSS so making NeoModal would make more sense

@floyd-li
Copy link
Member Author

floyd-li commented Apr 3, 2023

Hey, really nice job 🤗 .

Can I ask why you haven't used https://oruga.io/components/modal.html ? We are rewriting a lot of ui with Oruga and Custom CSS so making NeoModal would make more sense

i just saw we're use $buefy.modal.open for programmatic modal open previously. so i just follow it. maybe we need refactor next?

@exezbcz
Copy link
Member

exezbcz commented Apr 3, 2023

good job! 🚀

@vikiival
Copy link
Member

vikiival commented Apr 3, 2023

Hey, really nice job 🤗 .

Can I ask why you haven't used https://oruga.io/components/modal.html ? We are rewriting a lot of ui with Oruga and Custom CSS so making NeoModal would make more sense

i just saw we're use $buefy.modal.open for programmatic modal open previously. so i just follow it. maybe we need refactor next?

K lets do it in separated issue.
I find code really hard to read, but understand that this is really hacky solution. Happy to forward and see NeoModal soon 🤗

Waiting for @

@vikiival
Copy link
Member

vikiival commented Apr 3, 2023

Ahaha wanted to tag @exezbcz but gh crashed 🥲

@exezbcz
Copy link
Member

exezbcz commented Apr 3, 2023

@vikiival

@floyd-li
Copy link
Member Author

floyd-li commented Apr 3, 2023

K lets do it in separated issue. I find code really hard to read, but understand that this is really hacky solution. Happy to forward and see NeoModal soon 🤗

will make some changes after this PR get merged #5366

then it can be mergable

utils/constants.ts Outdated Show resolved Hide resolved
@codeclimate
Copy link

codeclimate bot commented Apr 3, 2023

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

View more on Code Climate.

@vikiival
Copy link
Member

vikiival commented Apr 3, 2023

Let's go

@vikiival
Copy link
Member

vikiival commented Apr 3, 2023

pay 35 usd

@vikiival vikiival merged commit 79db326 into kodadot:main Apr 3, 2023
16 checks passed
@yangwao
Copy link
Member

yangwao commented Apr 3, 2023

😍 Perfect, I’ve sent the payout
💵 $35 @ 32.62 USD/KSM ~ 1.073 $KSM
🧗 EZiu1PjV2j2JHKxY6mHnFwwCRCoV27uHKQSkKXATSh1srJT
🔗 0xfc1d44e1b5972cdac79f6928cdeee215c8446e915e44987f9dcc95240948b796

🪅 Let’s grab another issue and get rewarded!
🪄 github.com/kodadot/nft-gallery/issues

@yangwao yangwao added the paid pull-request has been paid label Apr 3, 2023
This was referenced Apr 7, 2023
@floyd-li floyd-li deleted the feat-redirect-external-link branch April 21, 2023 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paid pull-request has been paid S-visual-ok-✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Safety for external links
7 participants