Warn when closing window with unsaved changes #1230
Conversation
|
|
||
| let message = [ | ||
| padding(20), | ||
| paddingBottom(30) // I don't know why this is needed, but it is |
There was a problem hiding this comment.
There's some bugginess with the content size calculation here which causes the content to overflow unless adding some extra padding.
There was a problem hiding this comment.
Seems like the text wrapping is what's causing the issue here. If the text doesn't wrap then everything measures as expected.
There was a problem hiding this comment.
Ah, ya, it sounds like from our conversation there may be a bug with the measure method of the TextNode 🤔
| @@ -54,6 +54,7 @@ type t = { | |||
| pane: Pane.t, | |||
| searchPane: Feature_Search.model, | |||
| focus: Focus.stack, | |||
| modal: option(Modal.t), | |||
There was a problem hiding this comment.
We might want to combine the modal and context menu model, so that a modal and context menu can't both be visible at the same time. Have to merge the context menu PR before doing that though.
There was a problem hiding this comment.
Cool. I think the context menu should be unblocked now (#1232 brought in the boxShadow styling)
| | WindowCloseDiscardConfirmed | ||
| | WindowCloseSaveAllConfirmed |
There was a problem hiding this comment.
These are a bit awkward as they're essentially user intents. I also don't think it's necessarily a problem to have "action-like" messages, and with feature projects the encapsulation and lack of granularity might sometimes make event-like messages difficult. But enforcing event-like names is still very useful to stop thinking of them as functions.
This is also a reason why I'm leaning more towards using msg instead of event. Another reason being that the term is established and familiar from the Elm architecture.
There was a problem hiding this comment.
Sounds good! It seems like we're converging towards a very Elm-like architecture (ie, updater, subscription, and msg).
| let applyBufferUpdate = bufferUpdate => | ||
| ofBufferOpt(buffer => Buffer.update(buffer, bufferUpdate)); | ||
| Option.map(buffer => Buffer.update(buffer, bufferUpdate)); |
There was a problem hiding this comment.
Oh, nice, thanks for cleaning this up!
|
Cool to see this in action! Thanks for all the work on this, @glennsl |
Fixes #610
The message is a bit unbalanced, both visually and in terms of "scanability". It would be better to lead with a very short question summarizing the decision to be made, and then expand on it. We can tweak this later though, but it would be nice to have rich text support to be able to more easily to so.
A neat keyboard interface is planned for a subsequent PR.