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

fix: Fix transak and c14 icons and navigation #483

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

julia-script
Copy link
Contributor

@julia-script julia-script commented Aug 20, 2023

changes here are:

  1. switch next/link to a simple 'a'. The reason being, next link is intended for internal navigation, which means it will account for the basePath next config.
  2. replaced the icons for svg icons, basically cherry picked this from my other draft pr refactor: optmize build sizes by refactoring icons with inline pngs and marking some dependencies as peerDependencies #480
  3. added a convenience method to make navigation works locally too

Side note: there's a flag disabling the security/detect-object-injection. In my other pr that addresses eslint setup, I disabled that globally.
The reasons I think that should be disabled are:

  1. This rule is extremely strict. It basically disallow any way of accessing an objects prop dynamically even though there is no other way of doing that that would not trigger this error.
  2. The workaround suggested online is to replace something like obj[dynamicKey] by obj[`${dynamicKey}`]. This though is not any safer, this is probably just bypassing the lint rule check
  3. the author of the library himself says this here A more relevant "detect-object-injection" eslint-community/eslint-plugin-security#21 (comment)

I'm the original author of this rule - for a bit of context, it was originally written as an assistive tool for manual code reviews, to be used with the eslint plugin for VS Code. I would recommend disabling it for other use cases, as it's just going to be far too noisy.

If we don't want to completely disable that, I'd suggest having it enabled for vscode, but disabled at the actual check. Although, personally, I'd just disable it

@netlify
Copy link

netlify bot commented Aug 20, 2023

Deploy Preview for evmos-governance ready!

Name Link
🔨 Latest commit 35997be
🔍 Latest deploy log https://app.netlify.com/sites/evmos-governance/deploys/64e20ced5174c700081340ab
😎 Deploy Preview https://deploy-preview-483--evmos-governance.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.

@netlify
Copy link

netlify bot commented Aug 20, 2023

Deploy Preview for tubular-tarsier-72bc1a ready!

Name Link
🔨 Latest commit 35997be
🔍 Latest deploy log https://app.netlify.com/sites/tubular-tarsier-72bc1a/deploys/64e20cedd51631000828d097
😎 Deploy Preview https://deploy-preview-483--tubular-tarsier-72bc1a.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.

@codeclimate
Copy link

codeclimate bot commented Aug 20, 2023

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

View more on Code Climate.

@netlify
Copy link

netlify bot commented Aug 20, 2023

Deploy Preview for evmos-mission ready!

Name Link
🔨 Latest commit 35997be
🔍 Latest deploy log https://app.netlify.com/sites/evmos-mission/deploys/64e20ced77fb3c0008f814d5
😎 Deploy Preview https://deploy-preview-483--evmos-mission.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.

@netlify
Copy link

netlify bot commented Aug 20, 2023

Deploy Preview for evmos-assets ready!

Name Link
🔨 Latest commit 35997be
🔍 Latest deploy log https://app.netlify.com/sites/evmos-assets/deploys/64e20ced7623d60008f0f027
😎 Deploy Preview https://deploy-preview-483--evmos-assets.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.

@netlify
Copy link

netlify bot commented Aug 20, 2023

Deploy Preview for evmos-staking ready!

Name Link
🔨 Latest commit 35997be
🔍 Latest deploy log https://app.netlify.com/sites/evmos-staking/deploys/64e20cedd51631000828d09c
😎 Deploy Preview https://deploy-preview-483--evmos-staking.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.

@netlify
Copy link

netlify bot commented Aug 20, 2023

Deploy Preview for staging-vesting ready!

Name Link
🔨 Latest commit 35997be
🔍 Latest deploy log https://app.netlify.com/sites/staging-vesting/deploys/64e20ced6ca3290008c238ff
😎 Deploy Preview https://deploy-preview-483--staging-vesting.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.

@sandoche sandoche merged commit b791431 into main Aug 21, 2023
45 checks passed
@sandoche sandoche deleted the julia/icons-and-navigation-fix branch August 21, 2023 08:46
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.

None yet

2 participants