Desktop Modal Experiment #11484
Desktop Modal Experiment #11484
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #11484 +/- ##
==========================================
- Coverage 33.56% 33.53% -0.03%
==========================================
Files 2926 2928 +2
Lines 91527 91605 +78
Branches 17395 17412 +17
==========================================
- Hits 30721 30720 -1
- Misses 58392 58471 +79
Partials 2414 2414 ☔ View full report in Codecov by Sentry. |
|
No New Or Fixed Issues Found |
Co-authored-by: Anders Åberg <[email protected]>
abergs
force-pushed
the
passkey-window-creation
branch
from
595b431 to
a09a471
Compare
|
Ready for first review (but waiting on builds to finish, don't know if I've broken anything in the last commits) |
| const IN_MODAL_MODE = new KeyDefinition<boolean>(DESKTOP_SETTINGS_DISK, "inModalMode", { | ||
| deserializer: (b) => b, | ||
| }); |
There was a problem hiding this comment.
question: shouldn't this be stored in memory? Disk persists across process restarts?
There was a problem hiding this comment.
Good point, will do (any other opinion @justindbaur?)
There was a problem hiding this comment.
Right now this wouldn't work, memory storage isn't shared between renderer and main process right now. And I don't really want it to, at least not for this. We could maybe think about clearing it on start of the application though.
There was a problem hiding this comment.
I think that resolves this discussions, right?
There was a problem hiding this comment.
Ohh we need that setting to sync. Clearing it on application start should be enough, just to be safe so we don't introduce bugs if the application is force-closed in modal mode
There was a problem hiding this comment.
@abergs I still think we need to clear this value on application start
There was a problem hiding this comment.
Ah, I see! Newbie question: How do one go about doing that? @coroiu
There was a problem hiding this comment.
Wait - since the app can/should launch into modal mode, it's important we don't clear it and automatically throw the user out of modal mode. Perhaps that's not an issue, I don't know the implementation too well.
| const IN_MODAL_MODE = new KeyDefinition<boolean>(DESKTOP_SETTINGS_DISK, "inModalMode", { | ||
| deserializer: (b) => b, | ||
| }); |
There was a problem hiding this comment.
@abergs I still think we need to clear this value on application start
Tracking
https://bitwarden.atlassian.net/browse/PM-14223
📔 Objective
This PR aims to add a way for the desktop window to be presented in a smaller modal mode to enhance the UX for certain flows, like passkeys and SSH Agent.
Todo - but could just as well be separate PRs (are tracked in JIRA as subtasks)
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changesCo-authored-by: @justindbaur