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

Worksheet View UI Optimizations #4343

Merged
merged 34 commits into from
Dec 27, 2022
Merged

Worksheet View UI Optimizations #4343

merged 34 commits into from
Dec 27, 2022

Conversation

leilenah
Copy link
Collaborator

Major Changes

  1. Bundle rows no longer expand in place when you click the down arrow icon on a bundle row in the worksheet view. If you click the [new] expand icon on a bundle row, the bundle detail view will be swapped into the worksheet (see demo).
  2. The /worksheets/:uuid route now accepts an optional :bundle_uuid param.
  3. Initiating a new run from the worksheet view no longer opens the NewRun view in place. The NewRun view will now open in a modal.

Minor Changes

  1. A back button has been added to the worksheet view header.
  2. Hover state has been removed from tables in worksheet view. Now, table hover styles only change when a user hovers over a specific bundle row.
  3. Schema name field no longer defaults to an error state.

Related issues

#4300

Demo

20221223143055910.mp4

Checklist

  • I've added a screenshot of the changes, if this is a frontend change
  • I've added and/or updated tests, if this is a backend change
  • I've run the pre-commit.sh script
  • I've updated docs, if needed

@leilenah leilenah linked an issue Dec 23, 2022 that may be closed by this pull request
18 tasks
@leilenah
Copy link
Collaborator Author

@percyliang ready for your eyes 🙇

Copy link
Collaborator

@percyliang percyliang left a comment

Choose a reason for hiding this comment

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

Overall, this is great - really nice work! A few minor things that I ran into while playing with it:

  1. Could we make the New Run modal take up a bit more of the screen?
  2. If the user closes 'New Run', I wonder if it would make sense to keep the state around (in case they were typing a long command) so their work doesn't get lost? It doesn't complicate things, so not super opinionated about this, but thought I'd bring it up.
  3. 'Enter' doesn't work any more as a hot key to open up a bundleo
  4. Could we have 'u' go back to the worksheet page from the bundle view like in Gmail?
  5. When you go back to the worksheets page, it should keep the scroll exactly where you came from (currently, it scrolls to the top)
  6. When you hit rerun, it takes you to a bundle page, but the scroll position is not at the top (it should be)
  7. After hitting rerun, when hit 'Back', the cursor isn't anywhere (it should be on the bundle that we just came from)
  8. When you load a worksheet, a blue bar should show at the top of the worksheet to denote that the cursor is before any bundle (this used to be there but is gone now)
  9. When you hit 'gg' at the end of a worksheet that doesn't fit on one page, focus doesn't follow to the top of the worksheet (this was working before)

Some other bugs that I noticed (not related to this PR), but just putting them here for now:

  1. Keyboard shortcuts scrolls off the end of the page
  2. When you hold down 'j' or 'k' to scroll, the cursor position freezes instead of moving up/down the page gracefully as if you were in a text editor

@leilenah
Copy link
Collaborator Author

@percyliang

Could we make the New Run modal take up a bit more of the screen?

done

If the user closes 'New Run', I wonder if it would make sense to keep the state around (in case they were typing a long command) so their work doesn't get lost? It doesn't complicate things, so not super opinionated about this, but thought I'd bring it up.

Currently, the NewRun data isn't stored anywhere when that component is removed from DOM. I would prefer to keep that as-is, but I can re-work that view later if this ends up being an annoyance for users.

'Enter' doesn't work any more as a hot key to open up a bundle

fixed

Could we have 'u' go back to the worksheet page from the bundle view like in Gmail?

You can now use escape to go back to the worksheet page. I prefer escape over u because escape is what is currently used to un-collapse bundle details and I don't want to confuse users who have already gotten used to this. escape also seems more intuitive.

When you go back to the worksheets page, it should keep the scroll exactly where you came from (currently, it scrolls to the top)

fixed

When you hit rerun, it takes you to a bundle page, but the scroll position is not at the top (it should be)

fixed

After hitting rerun, when hit 'Back', the cursor isn't anywhere (it should be on the bundle that we just came from)

fixed

When you load a worksheet, a blue bar should show at the top of the worksheet to denote that the cursor is before any bundle (this used to be there but is gone now)

fixed

When you hit 'gg' at the end of a worksheet that doesn't fit on one page, focus doesn't follow to the top of the worksheet (this was working before)

fixed

Keyboard shortcuts scrolls off the end of the page

Are you referring to the Keyboards Shortcuts modal that pops open when you click the shortcut info icon? If so, you can scroll to see the rest of the info. It appears that this implementation was intentional.

When you hold down 'j' or 'k' to scroll, the cursor position freezes instead of moving up/down the page gracefully as if you were in a text editor

Hm. Not sure how useful this would be.

@percyliang
Copy link
Collaborator

This is great! Two more things I noticed:

  1. Now hitting 'G', which puts focus on the last bundle, doesn't scroll the page down to the bottom.
  2. Hitting 'escape' from a worksheet page now goes back, which is new / jarring, so probably we should prevent that from happening.

@leilenah
Copy link
Collaborator Author

@percyliang

Hitting 'escape' from a worksheet page now goes back, which is new / jarring, so probably we should prevent that from happening.

fixed

Now hitting 'G', which puts focus on the last bundle, doesn't scroll the page down to the bottom.

This works on my end:

20221226173045373.mp4

@percyliang
Copy link
Collaborator

Interesting, that page works for me too, but if you create a new worksheet and create one bundle and then replicate it 100x times (probably easiest to do Raw Edit and copy/paste in the editor), then I think this should reproduce the bug.

@leilenah
Copy link
Collaborator Author

@percyliang should be fixed now

@percyliang
Copy link
Collaborator

'G' works, but when afterwards when I hit 'gg', I get:
image

@leilenah
Copy link
Collaborator Author

@percyliang yikes 😅 fixed.

Copy link
Collaborator

@percyliang percyliang left a comment

Choose a reason for hiding this comment

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

Seems to work now. :-) Thanks for this great change - the interface feels so much more tight and natural!

@leilenah
Copy link
Collaborator Author

@percyliang so glad you like it! 😃

@leilenah leilenah merged commit 94b5ca0 into master Dec 27, 2022
@leilenah leilenah deleted the feature/4300-worksheet-view branch December 27, 2022 07:05
@epicfaace epicfaace mentioned this pull request Jan 18, 2023
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.

Optimize Worksheet View
2 participants