Notifications #365

FrigoEU · 2017-04-13T09:36:22Z

Hey,

This is a pretty big PR that corresponds to #98 most closely, in the future maybe to #39 and the design is stolen for the most part from atom/notifications#29. I started from the atom css and markup but ending up rendering it in a normal table.

I split up the commits in 3:

The first one is the biggest one, adding the actual component, hooking it up to Redux, adding layout, adding some examples etc.
The second adds some extra actions for showing/hiding, and adds a command to show the panel
The third implements the notifications in the NeovimInstance and the Config

This is not really finished yet, but far enough along to show I think. Here's some screenshots, mixing the example data and some actual notifications:

Use the new command "Show Notifications" to see it pop open. Note that the first few notifications are examples.

What still needs to happen:

Remove examples. I thought I'd leave them in if anyone wants to check
Refactor some more console statements. I figure this can be done in a later PR
The biggest: position the panel correctly. Right now I render it straight over the editor, hiding a good chunk of it. This is obviously not OK, but I'm not sure yet how to handle this in a robust way. I'd basically need to shrink and grow the .oni-text-editor when the changeNotificationVisibility actions come in, but this also comes into contact with UI: Externalize Window Management #362 so I thought I'd hold off on that just for now until somebody else can take a look at it.
Right now the only way you can view this new notifications panel is via a command. I think it would be nice to integrate it with Beginning external status line implementation #201 once it's ready.

Some other remarks on the code:

I think emitting some new events from the NeovimInstance was the best idea, to avoid tight coupling. I just followed along with how it already works.
Config was already an EventEmitter, but it wasn't emitting anything yet. I have it emit an event now + added a method to check for parsing errors. I wish I didn't have to do both, but because of how the Config initialises, you can never listen to the first parsing error being emitted so I "solved" it like this. Any input more than welcome.
I made the CSS and markup as simple and straightforward as I could, but I again introduced a bunch of variables etc that need to be part of some design system/theming solution/etc.

bryphe · 2017-04-13T19:10:23Z

Wow, this change looks awesome! Thanks a lot for including the screenshot. The UI looks really great and since it was inspired by Atom it has that modern UX feel.

This seems like it would be a convenient place for hosting the compile errors & warnings as well, at some point. The redux state changes look solid to me.

I added a few comments inline, and then just had these questions:

Is it possible to style the scrollbar that comes up? At least on Windows, it looks pretty out-of-place:
I'm wondering if the current component should be named Logs/LogRenderer? When I think of Notifications, I think of the pop-up notifications like:

I imagine we'll have the need for both styles of Notifications, so naming the component in the PR Logs will mean we can use Notifications for that pop-up experience.

Another thing I'm thinking about in the longer-term is how to keep Oni keyboard-friendly with these new components. Something like the vimium behavior might be really helpful - I created #366 to discuss this. Not something we need to tackle now but longer-term it'd be useful, especially as we add more of these rich UI experiences to Oni.

Excellent work, this looks great!

bryphe · 2017-04-13T19:11:29Z

browser/src/NeovimInstance.ts

@@ -97,7 +97,7 @@ export class NeovimInstance extends EventEmitter implements INeovimInstance {

        this._initPromise = startNeovim(this._pluginManager.getAllRuntimePaths(), filesToOpen)
            .then((nv) => {
-                console.log("NevoimInstance: Neovim started") // tslint:disable-line no-console
+                this.emit("logInfo", "NevoimInstance: Neovim started")


Would you mind fixing my typo while you're here? 😄 I used NevoimInstance instead of NeovimInstance

bryphe · 2017-04-13T19:11:48Z

browser/src/NeovimInstance.ts

@@ -326,7 +326,7 @@ export class NeovimInstance extends EventEmitter implements INeovimInstance {
        this._initPromise.then(() => {
            this._neovim.uiTryResize(columns, rows, (err?: Error) => {
                if (err) {
-                    console.error(err)
+                    this.emit("logError", err)


Thanks for hooking these up to the new logging infrastructure

bryphe · 2017-04-13T19:12:52Z

browser/src/UI/components/Notifications.less

+
+@component-padding: 10px;
+@border-color: rgb(25, 25, 25);
+// @font-family-monospace: 


Looks like there are a few unused lines here

bryphe · 2017-04-13T19:13:05Z

browser/src/UI/components/Notifications.less

+@import (reference) "./common.less";
+
+@icon-size: 30px;
+@font-family-monospace: Consolas, "Liberation Mono", Menlo, Courier, monospace;


This might be a good candidate to have in common.less - same for a few other constants.

bryphe · 2017-04-13T19:13:41Z

browser/src/UI/components/Notifications.tsx

+        }
+    }
+}
+function mapStateToProps(s: State.IState): Partial<INotificationsProps> {


Nice use of Partial here! I need to remember to leverage that..

…n.less

FrigoEU · 2017-04-14T07:20:45Z

I renamed it all to Log/Logs, it fits better indeed. I cleaned up some of the CSS + corrected display of long stack traces.

I don't have a windows PC at my disposal right now, so I didn't do the scrollbars yet.

bryphe · 2017-04-14T14:19:58Z

Awesome, thanks @FrigoEU ! This looks excellent. Appreciate you taking the time to do the rename... we need to get the rename IDE functionality in here to make that easier 😄

We can track the scrollbar issue separately, I'm happy to take a look at that since my main dev machine is Windows.

The only thing left is that we should remove the placeholder logs, could you please take those out? Otherwise I'm set to merge this in!

FrigoEU · 2017-04-14T17:22:12Z

Oki, I removed the examples.

Yes, it would be nice to track the scrollbars in a different issue. Thanks for any help you can give, otherwise I'll find some windows machine to run Oni on later.

bryphe · 2017-04-14T21:46:47Z

Awesome, thanks for addressing those! I'll open an issue for the windows scrollbar. I'm merging in now - thanks for the contribution, @FrigoEU !

bryphe · 2017-04-14T21:52:48Z

FYI - logged #370 to track the windows styling issues.

FrigoEU added 3 commits April 13, 2017 01:06

First version of notifications log

7360ebf

Adds showing and hiding of notifications panel

e40cac5

Refactored logs in NeovimInstance and alert in Config to notifications

7b28658

bryphe mentioned this pull request Apr 13, 2017

Keyboard Accessibilty for non-Vim/Neovim features #366

Closed

bryphe reviewed Apr 13, 2017

View reviewed changes

FrigoEU added 2 commits April 14, 2017 08:53

Rename notifications to logs

e7c0bf3

Improves styling of long stack traces and extracts variables to commo…

c91eb30

…n.less

Removes example logs

f172c15

bryphe merged commit 0b860a7 into onivim:master Apr 14, 2017

This was referenced Apr 14, 2017

Implement notification UX #98

Closed

UI - Logs: Style scrollbar for logs UI on Windows #370

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notifications #365

Notifications #365

FrigoEU commented Apr 13, 2017 •

edited

Loading

bryphe commented Apr 13, 2017

bryphe Apr 13, 2017

bryphe Apr 13, 2017

bryphe Apr 13, 2017

bryphe Apr 13, 2017

bryphe Apr 13, 2017

FrigoEU commented Apr 14, 2017 •

edited

Loading

bryphe commented Apr 14, 2017

FrigoEU commented Apr 14, 2017 •

edited

Loading

bryphe commented Apr 14, 2017

bryphe commented Apr 14, 2017

Notifications #365

Notifications #365

Conversation

FrigoEU commented Apr 13, 2017 • edited Loading

bryphe commented Apr 13, 2017

bryphe Apr 13, 2017

Choose a reason for hiding this comment

bryphe Apr 13, 2017

Choose a reason for hiding this comment

bryphe Apr 13, 2017

Choose a reason for hiding this comment

bryphe Apr 13, 2017

Choose a reason for hiding this comment

bryphe Apr 13, 2017

Choose a reason for hiding this comment

FrigoEU commented Apr 14, 2017 • edited Loading

bryphe commented Apr 14, 2017

FrigoEU commented Apr 14, 2017 • edited Loading

bryphe commented Apr 14, 2017

bryphe commented Apr 14, 2017

FrigoEU commented Apr 13, 2017 •

edited

Loading

FrigoEU commented Apr 14, 2017 •

edited

Loading

FrigoEU commented Apr 14, 2017 •

edited

Loading