Restyle wild menu/add commandline cursor #1191
Conversation
also add icon - file icon atm ?alternatives
Components now render in the same layer and can be positioned similarly if both present or interchangeably if one options is disabled
Also only render overlay if both elements present
|
Noted a bug with the I've added a delay to component mounting so the command executes and state shifts back to |
|
Nice, thanks for putting this together! The screenshot is looking great! I'll try it out shortly (I'm actually with family at the moment, so might be a bit delayed 😄 )
Happy holidays to you too! 🎄 Hope you're getting some time to relax 👍 |
|
@bryphe no worries taking some family-imposed R&R myself 😆 |
using whitespace: pre-wrap from one of see facebook/react#4134 html auto removes trailing white space breaking the cursor's position functionality
to ensure commandline and wild menu have same width
|
A though re. a further stylistic change that could come as part of or after this @bryphe could be using the value of the first char e.g. Also should note here that according to the nvim docs the command line can be opened recursively for example using |
| public handleChange(e: React.ChangeEvent<HTMLInputElement>) { | ||
| // UI.Actions.setCommandLinePosition(1, 1) | ||
| public componentDidMount() { | ||
| this.timer = setTimeout(() => { |
There was a problem hiding this comment.
Just curious - why do we need this timeout here? It feels a bit slow to me when I press the : key (like there is some perceptible delay) - I think it may be due to this?
I tried setting this to 0 and it seemed to work OK, which seemed to suggest it might not be necessary anymore - but it could also be the case that I missed the scenario in which it is needed 😄
There was a problem hiding this comment.
@bryphe it relates to a scenario where you have command line commands mapped and hit them in quick succession for example if you try map <tab> :bn<CR> and hit tab several times in a row with the timeout set to 0 the screen flickers as the component mounts very briefly. Its there to cover any/most (hopefully) of these scenarios.
There was a problem hiding this comment.
Whilst it does seem counter intuitive to try and hide it once I enabled it to test and tried using the functionality during a normal session I realised that as a vim user who has dozens of quick commandline mappings like :w or :b# etc. which I usually execute quickly and dont really think about having that quick flicker is really not a great experience though if you feel it detracts from the usability or have a suggestion other than the timeout like some sort of component mounting throttle I can remove it/try and implement something like that
There was a problem hiding this comment.
Oops, sorry I missed your other message. Yes, that quick flicker is painful - I didn't realize that it actually pops up through those command-line mappings. It's brutal especially if you use them frequently.
I think ideally, it would only show up via an explicit trigger, like when the user specifically presses :, \, ?. In the case of mappings, I'm not sure it makes sense to even show it - I didn't expect that behavior. I wonder if it would make sense to log an issue against Neovim for that case?
There was a problem hiding this comment.
Technically in neovim the same thing happens if you watch the command line, the command you entered appears so all the events we're hooked into occur
As for keypresses that sounds good, though one scenario I can see causing an issue is in the case of key remaps as in my case I've rebound : to ; so the command line wouldn't trigger and whilst I could just add that exception I dont know how many users would have done a similar thing and what their mappings might be for example \ remapped to | or something else which is fairly unpredictable or we could have it be that oni requires the default bindings or maybe instead offer config options to trigger the commandline if the user wants to remap this 🤔 for example default is ["/", "?", ":"] etc. and users can add keys to the config option.
There was a problem hiding this comment.
Though annoying given that it results in a delay a 80-100ms timeout seems to be enough unless a user literally held down a mapping for a good bit of time
|
Just got to try this out! It's looking great @Akin909 - thanks for all your work on this (not to mention, it was over the holidays!) I just had one question about the
Cool idea! I like it 👍 We could track it via a separate PR if you're up for it?
Ah yes I remember reading about this... I've never used the functionality either. Given that it works okay as it stands, and it might be a more advanced scenario, I'm okay holding off on implementing this for now - not a blocker IMO. Worst case, the user could always turn it off if they really don't like the behavior! If we complaints / issues we can figure out the best way to solve it at that time 😄 Thanks again - this is looking awesome! |
|
@bryphe I've reduced the timeout to 80ms which makes it feel much more responsive and for the most part doesn't cause the flicker unless a user hits the command very many times in very quick succession |
|
Sweet, sounds good! I'll go ahead and bring it in after the tests run 👍 It seems pretty usable now, so it'll be great to get feedback on it and see if there are any blockers to having it on by default. I'll make sure it to call it out in the next round of release notes! |
|
👍 Sounds great just pushed a change to re-run the tests which seem to be failing following the very tiny change I made which makes me think the failures might be sporadic and not related to the change which is very minor (seems to be the same tests that were failing in #1114. |
|
Sorry about the test failures - you're right, I don't believe they're related to your change. I think there is some issue with the yarn dependency caching on TravisCI that is causing these failures. I'll spend some more time investigating tomorrow (and worst case, I'll just double-check them locally and bring it in!) |
|
Just ran the tests locally and they worked - I think there is some sort of issue with the package caching. Locally, I had to clear out my |
|
Thanks for the contribution, @Akin909 ! Excited to start getting feedback on this - it's very cool 😄 |
|
@bryphe Thanks 😄 was pleased to work on this its so nice to see vim get prettier step by step ^^ |
@brype following the discussion in #1179 regarding the look of the wild menu, I've changed its styling to more closely resemble the quickCommand menu.
The wild menu now opens in a similar position with the same kind of styling or at least very close. I also added an icon which can be changed in the future its hard to know what is best here since the wild menu can be anything from vim commands to filenames, at the moment it's just a file icon which seemed sufficiently generic and a little less bland.
Lemme know what you think, also happy holidays ^^ 🎄
UPDATE: Also refactored the commandline component to give it an initial state like the other components with state, and added a sort of modal layer for both of the command line and wild menu so that they render in the same layer and can function either together or separately.