-
Notifications
You must be signed in to change notification settings - Fork 37
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
RFC: Implement a modal goto panel #34
Conversation
Looking good! RE the single-item issue, how about if the panel isn't shown at all before the promise is fulfilled? The loading indicator will show while it's waiting, but tbh it should barely matter since the request should basically always be instant. |
promise.then ((items) -> @view.setItems items), | ||
(error) -> @view.setError error | ||
|
||
class goToView extends SelectListView |
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.
This class name style sticks out a bit, doesn't it?
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.
True. Would GotoView
look better?
Yeah, sounds good. Where should the loading indicator be activated -- should that happen on a call to Also, thanks for the review! |
There should also be some unobstrusive mechanism for displaying errors and something like |
I tend to agree, but I don't think there's a realistic alternative right now. In future we might want to have some kind of status bar notification, but that's already getting pretty full. Something toast-like might be nice. I think |
e002e64
to
27809ac
Compare
Alright, this still needs specs, but should otherwise be good to go, I think. |
yes, that's much less elegant than before
I've rebased and updated the PR (and this and that as well). Yes, I realize the lack of tests is a bit hypocritical... :) |
Haha no worries. I think what makes sense is to do a big push for tests once things are a bit more stable. This looks good! Merge at your leisure :) |
also light refactoring
Pretty much this, except more general.
I'm not quire sure how to handle the case when only one item exists -- we can't know that before the promise is fulfilled, and that should arguably only happen after the goto-panel is displayed. It'd be a bit awkward to close it right after and jump straight to the def.
ToDo: