Skip to content

Add preference to disable table editor shortcuts and migrate preferences to localStorage #1901

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

Yukaii
Copy link
Member

@Yukaii Yukaii commented Mar 21, 2025

Introduce a new preference option to disable shortcuts in the table editor and migrate user preferences from cookies to localStorage for better management.

@Yukaii Yukaii added this to the Next milestone Mar 21, 2025
Signed-off-by: Yukai Huang <[email protected]>
@Yukaii Yukaii force-pushed the feature/dev-1881-shortcut-options branch 2 times, most recently from 2677d21 to 282ba60 Compare March 21, 2025 08:55
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new user preference to disable table editor shortcuts and migrates existing user preferences from cookies to localStorage for improved management.

  • Added a method to toggle table editor shortcuts in the table editor module.
  • Updated the statusbar UI to include a checkbox for disabling table editor shortcuts.
  • Implemented a Storage utility and a migration routine in the main index file to transition preference storage from cookies to localStorage.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
public/js/lib/editor/table-editor.js Adds a new API to enable/disable shortcuts and checks the saved cookie value.
public/js/lib/editor/statusbar.html Updates the dropdown menu to include a control for table editor shortcuts.
public/js/lib/editor/index.js Implements a Storage utility and migrates preferences from cookies to localStorage.

Comment on lines +144 to +147
// Check cookie for saved preference
const cookieDisableTableShortcuts = window.Cookies && window.Cookies.get('preferences-disable-table-shortcuts')
if (cookieDisableTableShortcuts && cookieDisableTableShortcuts === 'true') {
shortcutsEnabled = false
Copy link
Preview

Copilot AI Apr 18, 2025

Choose a reason for hiding this comment

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

Consider using the new Storage utility to retrieve the 'preferences-disable-table-shortcuts' value instead of directly calling window.Cookies, ensuring consistency after migration.

Suggested change
// Check cookie for saved preference
const cookieDisableTableShortcuts = window.Cookies && window.Cookies.get('preferences-disable-table-shortcuts')
if (cookieDisableTableShortcuts && cookieDisableTableShortcuts === 'true') {
shortcutsEnabled = false
// Check storage for saved preference
const storageDisableTableShortcuts = Storage && Storage.get('preferences-disable-table-shortcuts')
if (storageDisableTableShortcuts && storageDisableTableShortcuts === 'true') {

Copilot uses AI. Check for mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this block is not necessary. Since this option is only added after the storage migration, we wouldn't need to check the cookies for this key. And because this option is handled in the Editor class, we don't need to check it here either:

var storedDisableTableShortcuts = Storage.get(
'preferences-disable-table-shortcuts'
)
if (storedDisableTableShortcuts && storedDisableTableShortcuts === 'true') {
disableTableShortcuts.prop('checked', true)
} else {
disableTableShortcuts.prop('checked', false)
}
this.setTableShortcutsPreference()

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.

3 participants