Skip to content
This repository was archived by the owner on Apr 1, 2020. It is now read-only.

Conversation

@akinsho
Copy link
Member

@akinsho akinsho commented Aug 28, 2018

Based off of #2522 (that should go in first) this PR adds a section with the available sessions to the welcome screen as well as the ability to restore a session.

This PR also has an updated welcome screen design (I begged a colleague who is a UI designer for some ideas for this hopefully we didn't got in the wrong direction)

screen shot 2018-08-28 at 20 54 00

akinsho added 30 commits August 22, 2018 10:25
fix navigation of commands
create common getBorder function
move quick commands to the top of the welcome screen
fix getBorder function to return default border of same size
re-arrange buttons for initial positioning
fix accidentally triggered session restoration
@badosu
Copy link
Collaborator

badosu commented Aug 28, 2018

Wow, that looks amazing! 👏

@codecov
Copy link

codecov bot commented Aug 28, 2018

Codecov Report

Merging #2527 into master will increase coverage by 0.08%.
The diff coverage is 53.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2527      +/-   ##
==========================================
+ Coverage   44.43%   44.51%   +0.08%     
==========================================
  Files         351      351              
  Lines       14260    14302      +42     
  Branches     1862     1865       +3     
==========================================
+ Hits         6336     6367      +31     
- Misses       7697     7709      +12     
+ Partials      227      226       -1
Impacted Files Coverage Δ
browser/src/UI/components/common.ts 92.85% <ø> (ø) ⬆️
browser/src/Services/Sessions/SessionManager.ts 74.19% <0%> (+0.86%) ⬆️
browser/src/Services/Metadata.ts 35.29% <22.22%> (-11.77%) ⬇️
browser/src/Plugins/Api/Oni.ts 37.63% <50%> (+0.27%) ⬆️
...ser/src/Editor/NeovimEditor/WelcomeBufferLayer.tsx 55.39% <61.29%> (+1.47%) ⬆️
...er/src/Editor/NeovimEditor/NeovimEditorCommands.ts 11.76% <0%> (-0.18%) ⬇️
browser/src/Services/Diagnostics.ts
browser/src/Services/Diagnostics/index.ts 39.02% <0%> (ø)
browser/src/Services/Sessions/SessionsStore.ts 33.33% <0%> (+3.57%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e612eb8...4a2b29e. Read the comment docs.

@akinsho akinsho force-pushed the feature/add-sessions-to-welcome branch from 62e8c10 to 207c42a Compare August 28, 2018 22:35
</Column>
<Column alignment="flex-start">
<SubtitleText>{`v${this.state.version}`}</SubtitleText>
{version && <SubtitleText>{`v${version}`}</SubtitleText>}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change doesnt prevent the whole welcome layer from rendering if there is an issue with the getMetadata function not sure how it could fail but I don't think anything other than a full editor/app crash should prevent the initial welcome page from rendering

@akinsho akinsho requested review from CrossR, TalAmuyal and bryphe August 29, 2018 15:57
@CrossR
Copy link
Member

CrossR commented Aug 29, 2018

Functionally, loading sessions works great! I like the redesign too, but a few points:

  • The header text ("Quick Commands"/ "Learn"/"Sessions") is a bit smaller than I'd expect, so it doesn't stand out from the contents below it that much.
  • The gap between the two sections (the menu + sessions) is pretty big (at least on my monitor).

Image for reference:

image

@akinsho
Copy link
Member Author

akinsho commented Aug 29, 2018

@CrossR thanks for having a look much appreciated 👍

re. the heading sizes I can increase those a bit for legibility
re. the spacing the idea was to have some spacing but between them and a visible divider, with your current theme it seems less obvious, but either way the gap may be too big there (as always im on a 15' mac so not the same effect, ill add a max width so that box is never too big so you dont get the large gap)

seperated selectedItem selected styles into css block
add margin to icon styles
@akinsho
Copy link
Member Author

akinsho commented Aug 29, 2018

@CrossR just tweaked the styles to reduce the overall size of the box, and increase the size of the headers, can you give it a try and let me know if its improved, locally the change is less obvious, also if the text is still to small can you see if tweaking you ui.fontSize changes it.

Copy link
Member

@CrossR CrossR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now!

There is still a few bits we could move around on the Welcome screen and tidy up, which I've commented on, but this PR hasn't changed that so feels a bit unfair to have you fix it here.

},
openFile: { command: "oni.editor.newFile" },
openWorkspaceFolder: { command: "workspace.openFolder" },
commandPalette: { command: "quickOpen.show" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is called commandPalette but opens "quickOpen.show", instead of "commands.show".

openFile: { command: "oni.editor.newFile" },
openWorkspaceFolder: { command: "workspace.openFolder" },
commandPalette: { command: "quickOpen.show" },
commandline: { command: "executeVimCommand" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"executeVimCommand" doesn't exist, "editor.executeVimCommand" does, but is used to execute a command, not show the cmdline bar which I assume this was intended for.

openTutor: { command: "oni.tutor.open" },
openDocs: { command: "oni.docs.open" },
openConfig: { command: "oni.config.openUserConfig" },
openThemes: { command: "oni.themes.open" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"oni.themes.open" doesn't exist, I assume this should instead be "oni.themes.choose".

@CrossR
Copy link
Member

CrossR commented Aug 30, 2018

(I realise the things I'm commenting on are just formatting changes, but just documenting it!)

@akinsho
Copy link
Member Author

akinsho commented Aug 30, 2018

@CrossR thanks for checking those I'll merge this and then open a smaller pr to fix up the other bits 👍

@akinsho akinsho merged commit 5beff5f into onivim:master Aug 30, 2018
@akinsho akinsho deleted the feature/add-sessions-to-welcome branch August 30, 2018 14:03
@akinsho akinsho restored the feature/add-sessions-to-welcome branch October 1, 2018 09:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants