-
Notifications
You must be signed in to change notification settings - Fork 46
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
chore(web): remove-unused-libs-and-update-contact-support-link #1733
chore(web): remove-unused-libs-and-update-contact-support-link #1733
Conversation
WalkthroughThe pull request includes modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Code Climate has analyzed commit 9e58743 and detected 0 issues on this pull request. View more on Code Climate. |
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/src/pages/Settings/EmailConfirmation/index.tsx (2)
101-101
: Consider security and UX improvements for external linksWhen redirecting users to external sites (Telegram), consider these improvements:
- Open links in new tab to preserve user context
- Add visual indication that links are external
- Consider adding a warning/confirmation before leaving the site
- buttonTo: "https://t.me/kleros", + buttonTo: { + href: "https://t.me/kleros", + external: true + },Then update the Link component to handle external URLs:
const ExternalAwareLink: React.FC<{ to: string | { href: string; external: boolean }; children: React.ReactNode }> = ({ to, children }) => { if (typeof to === 'object' && to.external) { return <a href={to.href} target="_blank" rel="noopener noreferrer">{children}</a>; } return <Link to={to}>{children}</Link>; };Also applies to: 109-109
177-179
: LGTM! Navigation implementation follows React Router best practicesThe change from imperative to declarative navigation is a good improvement. However, consider using the suggested
ExternalAwareLink
component from the previous comment to handle both internal and external navigation consistently.- <Link to={buttonTo}> + <ExternalAwareLink to={buttonTo}> <Button text={buttonMsg} /> - </Link> + </ExternalAwareLink>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (2)
web/package.json
(0 hunks)web/src/pages/Settings/EmailConfirmation/index.tsx
(3 hunks)
💤 Files with no reviewable changes (1)
- web/package.json
🔇 Additional comments (2)
web/src/pages/Settings/EmailConfirmation/index.tsx (2)
4-4
: LGTM! Import changes align with navigation updates
The addition of Link
import supports the move to declarative navigation, and useSearchParams
import properly declares a previously implicit dependency.
4-4
: Verify similar navigation patterns in the codebase
Let's check if there are other components that might benefit from the same navigation pattern updates.
Also applies to: 101-101, 109-109, 177-179
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.
lgtm
PR-Codex overview
This PR focuses on updating the
package.json
dependencies by removing certain packages, changing some dependencies, and modifying theEmailConfirmation
component to use aLink
instead of aButton
for navigation.Detailed summary
@netlify/functions
,@types/amqplib
,@filebase/client
,@middy/core
,@middy/http-json-body-parser
,amqplib
,siwe
.buttonTo
inEmailConfirmation
from/
tohttps://t.me/kleros
.EmailConfirmation
fromButton
withonClick
to aLink
.Summary by CodeRabbit
New Features
Chores