Skip to content

feat: add ctrl + arrow key to jump to first/last cell; fix left/right key navigation with hidden columns#1827

Merged
mtrezza merged 11 commits intoparse-community:masterfrom
404-html:key-navigation-hidden-columns-fix
Oct 5, 2021
Merged

feat: add ctrl + arrow key to jump to first/last cell; fix left/right key navigation with hidden columns#1827
mtrezza merged 11 commits intoparse-community:masterfrom
404-html:key-navigation-hidden-columns-fix

Conversation

@404-html
Copy link
Copy Markdown
Member

@404-html 404-html commented Oct 4, 2021 •
edited
Loading

New Pull Request Checklist

Issue Description

This merge request is about following:

  1. Fix for cell selection going to different dimension when navigating horizontally and having some columns hidden;

parse-hidden-columns

  1. Adding support for ctrl+left/right keys. With such combination navigation jumps to first/last visible column;

TODOs before merging

  • Add tests
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

@parse-github-assistant
Copy link
Copy Markdown

parse-github-assistant Bot commented Oct 4, 2021 •
edited
Loading

Thanks for opening this pull request!

  • ❌ Please edit your post and use the provided template when creating a new pull request. This helps everyone to understand your post better and asks for essential information to quicker review the pull request.

@404-html 404-html changed the title Left/Right key navigation - hidden columns fix + ctrl key support fix: Left/Right key navigation - hidden columns fix + ctrl key support Oct 4, 2021
@mtrezza mtrezza changed the title fix: Left/Right key navigation - hidden columns fix + ctrl key support fix: left/right key navigation with hidden columns, add ctrl + arrow key to jump to first/last cell Oct 4, 2021
@mtrezza
Copy link
Copy Markdown
Member

mtrezza commented Oct 4, 2021

I reworded the PR title slightly so that others reading the changelog have a better idea of the fix

@mtrezza
Copy link
Copy Markdown
Member

mtrezza commented Oct 4, 2021

Please feel free to request a review when this is ready.

@404-html
Copy link
Copy Markdown
Member Author

404-html commented Oct 4, 2021

Hey @mtrezza, thank you for your changes. I believe it's ready to be reviewed, please proceed.

mtrezza
mtrezza reviewed Oct 4, 2021
View reviewed changes
Comment thread src/dashboard/Data/Browser/DataBrowser.react.js Outdated
mtrezza
mtrezza reviewed Oct 4, 2021
View reviewed changes
Comment thread src/dashboard/Data/Browser/DataBrowser.react.js Outdated
@mtrezza mtrezza changed the title fix: left/right key navigation with hidden columns, add ctrl + arrow key to jump to first/last cell feat: add ctrl + arrow key to jump to first/last cell; fix left/right key navigation with hidden columns Oct 4, 2021
@mtrezza
Copy link
Copy Markdown
Member

mtrezza commented Oct 4, 2021 •
edited
Loading

Feature tested and works great!

Do you think you could also add the ctrl + arrow feature for jumping to the first / last row in the current column? I found this always to be tedious, currently you have to scroll all the way to the top.

@404-html
Copy link
Copy Markdown
Member Author

404-html commented Oct 5, 2021 •
edited
Loading

Thanks for testing @mtrezza. I've added support for ctrl+up/down but I'm afraid it still needs some work.
While it works fine with small data sets it's not fully reliable when lazy-loading comes into place - auto scroll is not working. The reason behind this is that with large data sets not all rows are rendered (probably for performance reasons) and code responsible for scrolling to the current row is in BrowserCell component (componentDidUpdate lifecycle method).
So when, let's say, we are trying to jump from first to last (not rendered) row scrolling logic will not execute.
Changing this performance optimization to render all rows just for this feature is not reasonable.

Another thing I tried is to watch for current row change in BrowserTable -> componentWillReceiveProps and when it's first or last - scroll up or down to the bottom. Best way here would be to implement some smart logic which will perform scroll only when needed. Anyway this change was not working reliably and it was quite slow, feels like all rows between were rendered which is not efficient.

My preference would be to abstract all scrolling logic into single class as having it now split between BrowserTable and BrowserCell (I'm not sure about other places) makes it a bit hard to not overlap.

I should have some time to work on that next week, or we can merge that as it is and raise new issue for ctrl+up/down + lazy loading optimization. Do you have any preference?

@mtrezza
Copy link
Copy Markdown
Member

mtrezza commented Oct 5, 2021 •
edited
Loading

Thanks for the thorough analysis!

I should have some time to work on that next week, or we can merge that as it is and raise new issue for ctrl+up/down + lazy loading optimization. Do you have any preference?

I think this already provides great value as it currently is. The scrolling refinement maybe can be fixed as a patch in a separate PR, so we can already make some developers happy with the added left/right jumping feature. And maybe we get more feedback in the meantime. Normally this would go into an alpha release, but we are still setting up the release process, so I would just stable-release it for now. Would that be fine with you?

@404-html
Copy link
Copy Markdown
Member Author

404-html commented Oct 5, 2021

Yes, thats fine with me. Thank you for asking 🤝

mtrezza
mtrezza approved these changes Oct 5, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@mtrezza mtrezza left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good! Thanks for this improvement to speed up the workflow in Parse Dashboard!

@mtrezza mtrezza merged commit b504c0f into parse-community:master Oct 5, 2021
parseplatformorg pushed a commit that referenced this pull request Oct 5, 2021
@semantic-release-bot
chore(release): 3.2.0 [skip ci]
7c56b75 
# [3.2.0](3.1.2...3.2.0) (2021-10-05)

### Features

* add ctrl + arrow key to jump to first/last cell; fix left/right key navigation with hidden columns ([#1827](#1827)) ([b504c0f](b504c0f))
@parseplatformorg
Copy link
Copy Markdown
Contributor

parseplatformorg commented Oct 5, 2021

🎉 This pull request has been released in version 3.2.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Oct 5, 2021
beiguancyc pushed a commit to beiguancyc/parse-dashboard that referenced this pull request Oct 9, 2021
@cycqitianwushuge
Merge branch 'source'
5c09465 
* source: (53 commits)
  chore(release): 3.2.1 [skip ci]
  chore(release): 3.2.1-beta.1 [skip ci]
  ci: fix prerelease labels
  chore(release): 3.2.1-alpha.1 [skip ci]
  fix: enabling context menu for read-only cells (parse-community#1844)
  docs: add info about --dev parameter (parse-community#1842)
  build: merge beta (parse-community#1841)
  build: merge alpha (parse-community#1840)
  docs: fix release changelog filename
  docs: reword changelog quote
  docs: fix changelog branch names (parse-community#1837)
  refactor: simplify reading dashboard config from a json file (parse-community#1828)
  ci: update release branch names
  chore(release): 3.2.0 [skip ci]
  feat: add ctrl + arrow key to jump to first/last cell; fix left/right key navigation with hidden columns (parse-community#1827)
  refactor: upgrade inquirer from 8.1.2 to 8.1.3 (parse-community#1829)
  refactor: upgrade otpauth from 7.0.5 to 7.0.6 (parse-community#1830)
  refactor: replace create-react-class with ES6 classes (parse-community#1818)
  refactor: replace query-string with URLSearchParams (parse-community#1819)
  docs: fix typo in refactor changelog entry
  ...

# Conflicts:
#	package-lock.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mtrezza mtrezza mtrezza approved these changes

Assignees

No one assigned

Labels

state:released Released as stable version

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@404-html @mtrezza @parseplatformorg