Skip to content
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

Merged
merged 11 commits into from
Apr 24, 2016
Merged

RFC: Implement a modal goto panel #34

merged 11 commits into from
Apr 24, 2016

Conversation

pfitzseb
Copy link
Member

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:

  • Specs!

@MikeInnes
Copy link
Member

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
Copy link
Member

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?

Copy link
Member Author

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?

@pfitzseb
Copy link
Member Author

pfitzseb commented Feb 1, 2016

Yeah, sounds good. Where should the loading indicator be activated -- should that happen on a call to goto or should it be the responsibility of the caller? I've gone with the latter option for now, but should be easy to change.

Also, thanks for the review!

@pfitzseb
Copy link
Member Author

pfitzseb commented Feb 1, 2016

There should also be some unobstrusive mechanism for displaying errors and something like No definitions found. on items.length == 0. Do we already have something like that? I'd rather not use the default Atom notifications, they're too much of an eyecatcher...

@MikeInnes
Copy link
Member

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 no definitions found == no response would be OK for now, so long as it's completely predictable.

@pfitzseb
Copy link
Member Author

Alright, this still needs specs, but should otherwise be good to go, I think.

@pfitzseb
Copy link
Member Author

I've rebased and updated the PR (and this and that as well).
Whaddaya think of merging, @MikeInnes?

Yes, I realize the lack of tests is a bit hypocritical... :)

@MikeInnes
Copy link
Member

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 :)

@pfitzseb pfitzseb merged commit ea0ecfa into master Apr 24, 2016
@pfitzseb pfitzseb deleted the va/gotodef branch April 24, 2016 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants