-
Notifications
You must be signed in to change notification settings - Fork 53
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
Small refactor to import panel #1194
Conversation
briehl
commented
Oct 27, 2017
- Cleaned up some things that made my linter angry - mainly whitespace and quotes.
- Changed staging area viewer to use newer API calls for handling things (fetching files, inserting app cells).
- Tweaked the file staging client to be a little more flexible with its list function.
@thomasoniii - let's get this merged before your changes start to conflict too much. |
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.
I'm leery of the catch that just returns an empty array w/o noting it at all on 74-77 of fileStaging. Should it at least have a console.error as well? Or is it truly a non-issue and it just gets reported as a failure when nothing's there? I'm not leery enough to hold it up, just to note it.
I have no idea why javascript linters are so insistent about single quotes. Perl? Sure, differentiate. But JavaScript just seems utterly arbitrary.
I see now. I put that no-op error there just as a convenience. I had it tooled together that if a list call fails, it should show an empty list. But you're right, if an error occurs there should at least be a warning. Actually, it should be up to whatever invokes the client to catch the error and deal with it there. So, really, the client should just pass that through. I'll make the adjustment. |