implemented login cancelled modal #2206
Conversation
|
|
WalkthroughAdds a new translation group and a LitElement Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant URL as Browser URL Hash
participant Main as Client (Main)
participant LCM as LoginCancelledModal
participant AM as AccountModal
U->>URL: Navigate with #login=cancel
URL->>Main: Hash change detected
Main->>LCM: open()
LCM-->>U: Render "login cancelled" modal
alt User clicks "Try again"
U->>LCM: Click Try again
LCM->>LCM: close()
LCM->>AM: find + open()
AM-->>U: Show account modal
else User clicks "Cancel"
U->>LCM: Click Cancel
LCM->>LCM: close()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Potential attention points:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/client/LoginCancelledModal.ts (3)
10-13: Consider extracting the modal element type.While the inline type definition works, you could extract it to a shared interface if multiple components need to reference the modal element type. This is optional.
Example:
interface ModalElement extends HTMLElement { open: () => void; close: () => void; } @query("o-modal") private modalEl!: ModalElement;
15-17: Remove unnecessary empty constructor.The constructor only calls
super()without additional logic. In TypeScript, this is unnecessary and can be removed. The parent constructor is automatically called.- constructor() { - super(); - } -
70-73: Consider removing async modifier and clarify requestUpdate usage.The
open()method is markedasyncbut doesn't await anything. Unless you plan to add async operations later, theasynckeyword can be removed.Additionally,
requestUpdate()is typically used when reactive properties change. Since you're only callingmodalEl.open(), verify ifrequestUpdate()is necessary here.- public async open() { + public open() { this.modalEl?.open(); - this.requestUpdate(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/lang/en.json(1 hunks)src/client/LoginCancelledModal.ts(1 hunks)src/client/Main.ts(4 hunks)src/client/index.html(1 hunks)
🔇 Additional comments (4)
resources/lang/en.json (1)
790-798: LGTM! Clean translation structure.The translation keys are well-organized and follow the existing naming conventions. The benefit messages are clear and the button labels are concise.
src/client/LoginCancelledModal.ts (1)
46-47: Verify the empty span in the divider.The divider contains an empty
<span>element with background styling. If this is purely decorative, consider adding anaria-hidden="true"attribute for accessibility or removing it if it serves no purpose.src/client/index.html (1)
402-402: LGTM! Consistent placement.The modal element is correctly positioned alongside other modal components. The tag name matches the custom element definition.
src/client/Main.ts (1)
251-259: LGTM! Consistent modal initialization.The initialization follows the established pattern used for other modals in the codebase. The instance check and warning message are helpful for debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/client/LoginCancelledModal.ts (3)
15-17: Remove the empty constructor.An empty constructor that only calls
super()with no arguments is unnecessary and can be removed.- constructor() { - super(); - } -
70-73: Remove async or add await if needed.The
open()method is declaredasyncbut doesn't await anything. Either removeasyncor document why it's needed for future compatibility.- public async open() { + public open() { this.modalEl?.open(); this.requestUpdate(); }
4-5: Remove unused component registrations. The imports for./components/Difficultiesand./components/PatternButtononly register<difficulty-display>and<pattern-button>, but neither tag is used in this template. Drop these imports or move them to a shared entry-point.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/LoginCancelledModal.ts(1 hunks)src/client/Main.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/Main.ts
🔇 Additional comments (2)
src/client/LoginCancelledModal.ts (2)
79-88: Great job addressing the previous review comment!The null check for
AccountModalhas been properly added, preventing runtime errors if the element is missing. This handles the concern raised in the previous review.
19-21: Document or revert disabled Shadow DOM
OverridingcreateRenderRoot()to returnthisremoves style encapsulation and breaks Shadow-DOM features (scoped CSS,:host, slots, event retargeting). If you need global styles (e.g. Tailwind) to apply here, add a comment explaining why; otherwise remove this override to restore isolated Shadow DOM.
|
This pull request is stale because it has been open for fourteen days with no activity. If you want to keep this pull request open, add a comment or update the branch. |
github-actions
bot
added
the
Stale
|
Could you resolve conflicts? |
TheGiraffe3
removed
the
Stale
Conflict resolved |
3 participants
Description:
I’ve implemented a modal that appears when #login=cancel is present in the URL after login through discord is cancelled. The modal explains the benefits of being logged in and asks the user if they’d like to try again. If they choose to retry, the current modal closes and the login modal opens again.
Solves: #1031
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
dthlss