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.

Sign up for GitHub

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

Jump to bottom

Implement back and forth navigation #9301

Merged
merged 13 commits into from
Mar 22, 2024
Merged

Implement back and forth navigation #9301

merged 13 commits into from
Mar 22, 2024

Conversation

MrFlashAccount
Copy link
Contributor

@MrFlashAccount MrFlashAccount commented Mar 6, 2024 •
edited by somebody1234
Loading

Pull Request Description

  • Closes enso-org/cloud-v2#940

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@MrFlashAccount MrFlashAccount self-assigned this Mar 6, 2024
@somebody1234
Copy link
Contributor

note that opening a PR asap isn't necessary - if anything we might even prefer opening PRs later (if not as late as possible) because each push takes up CI time, even if the PR is in draft state

(branches are ok though)

@MrFlashAccount
Copy link
Contributor Author

MrFlashAccount commented Mar 6, 2024 •
edited
Loading

note that opening a PR asap isn't necessary - if anything we might even prefer opening PRs later (if not as late as possible) because each push takes up CI time, even if the PR is in draft state

(branches are ok though)

yea, even though I opened the PR recently, I push only when I feel it's necessary :)

@MrFlashAccount MrFlashAccount force-pushed the wip/sergeigarin/add-back-and-forth-navigation branch from a74c269 to 13b3508 Compare March 8, 2024 14:32
@MrFlashAccount MrFlashAccount marked this pull request as ready for review March 11, 2024 10:26
somebody1234
somebody1234 reviewed Mar 11, 2024
View reviewed changes
app/ide-desktop/lib/dashboard/src/configurations/inputBindings.ts Outdated Show resolved Hide resolved
app/ide-desktop/lib/dashboard/src/hooks/eventCallback.ts Outdated Show resolved Hide resolved
app/ide-desktop/lib/dashboard/src/hooks/searchParamsState.ts Outdated Show resolved Hide resolved
app/ide-desktop/lib/dashboard/src/hooks/useLazyMemo.ts Outdated Show resolved Hide resolved
app/ide-desktop/lib/dashboard/src/hooks/useLazyMemo.ts Outdated Show resolved Hide resolved
app/ide-desktop/lib/dashboard/src/hooks/searchParamsState.ts Outdated Show resolved Hide resolved
app/ide-desktop/lib/dashboard/src/pages/dashboard/Dashboard.tsx Show resolved Hide resolved
app/ide-desktop/lib/dashboard/src/utilities/safeJsonParse.ts Outdated Show resolved Hide resolved
app/ide-desktop/lib/dashboard/src/pages/dashboard/Dashboard.tsx
)

React.useEffect(() => {
if (detect.isOnElectron()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about in the browser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keyboard navigation in the browser works out of the box

Copy link
Contributor

Choose a reason for hiding this comment

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

but Cmd+[ and Cmd+ArrowLeft do not - and i think users would be quite surprised when they see it in the shortcuts configuration page but somehow they do not work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, could you elaborate? I don't understand what you were trying to say by but Cmd+[ and Cmd+ArrowLeft do no

Copy link
Contributor

Choose a reason for hiding this comment

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

afaict cmd+[ is not a native browser shortcut (unless it is on macos).
similarly cmd+arrowleft is a native browser shortcut on neither windows nor linux

@somebody1234
Copy link
Contributor

is it not an option to useSearchParamsState somewhere where we have setBackendType?

@PabloBuchu
Copy link
Contributor

qa 🟢

@PabloBuchu
Copy link
Contributor

missing no changelog required label

somebody1234
somebody1234 reviewed Mar 11, 2024
View reviewed changes
app/ide-desktop/lib/dashboard/src/hooks/eventCallback.ts Outdated Show resolved Hide resolved
app/ide-desktop/lib/dashboard/src/hooks/eventCallback.ts Outdated Show resolved Hide resolved
app/ide-desktop/lib/dashboard/src/hooks/searchParamsState.ts Outdated Show resolved Hide resolved
app/ide-desktop/lib/dashboard/src/pages/dashboard/Dashboard.tsx
)

React.useEffect(() => {
if (detect.isOnElectron()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

but Cmd+[ and Cmd+ArrowLeft do not - and i think users would be quite surprised when they see it in the shortcuts configuration page but somehow they do not work

app/ide-desktop/lib/dashboard/src/hooks/eventCallback.ts Outdated Show resolved Hide resolved
@MrFlashAccount MrFlashAccount added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 12, 2024
@somebody1234
Copy link
Contributor

btw: i think the event handler is still needed for browser, because Alt+[ and Alt+] are not natively supported by windows/linux

@somebody1234
Copy link
Contributor

also the icons for "go back" and "go forward" seem to be 24px, making the icons column in the "keyboard shortcuts" settings to be styled a bit awkwardly - note that most of our icons are 16px

@MrFlashAccount MrFlashAccount force-pushed the wip/sergeigarin/add-back-and-forth-navigation branch from 53cf827 to 2db1f06 Compare March 18, 2024 13:41
indiv0
indiv0 approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@indiv0 indiv0 left a comment

Choose a reason for hiding this comment

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

LGTM

app/ide-desktop/lib/dashboard/src/utilities/safeJsonParse.ts
const parsed: unknown = JSON.parse(value)
return predicate(parsed) ? parsed : defaultValue
} catch {
return defaultValue
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we want to log if we're unable to parse the JSON for some reason? Just for debugging purposes, if we start to get defaultValue when we don't expect to.

@MrFlashAccount MrFlashAccount force-pushed the wip/sergeigarin/add-back-and-forth-navigation branch from edf7002 to 1de0985 Compare March 19, 2024 13:49
@MrFlashAccount
Copy link
Contributor Author

btw: i think the event handler is still needed for browser, because Alt+[ and Alt+] are not natively supported by windows/linux

Windows(and linux) natively support only Alt + arrow, thus I'm not sure it's a good idea to provide different "not native" shortcuts for the platform. We added Custom keybindings for electron only because it doesn't support back&forth navigation at all.

@somebody1234
Copy link
Contributor

while that's true, they still show up on the keyboard shortcuts page, meaning the user would expect them to actually work.
if that's not intended, then we will want to either:

  • hide them from the keyboard shortcuts page, or
  • do more feature detection to disable the non-native shortcuts in browser environments. (however. this means that the shortcuts do still need to be handled.)

also note that it will probably be surprising for behavior to be different between electron and browser.

@somebody1234 somebody1234 added x-new-feature Type: new feature request g-dashboard labels Mar 20, 2024
@MrFlashAccount
Copy link
Contributor Author

Yea, I see you point, actually it's a mistake on my side (we shouldn't have Alt+] on windows/linux). Removed it from the keybindings

@MrFlashAccount MrFlashAccount added the CI: Ready to merge This PR is eligible for automatic merge label Mar 20, 2024
@MrFlashAccount MrFlashAccount force-pushed the wip/sergeigarin/add-back-and-forth-navigation branch from 8be863b to 6126f85 Compare March 21, 2024 05:59
@MrFlashAccount MrFlashAccount force-pushed the wip/sergeigarin/add-back-and-forth-navigation branch from 6126f85 to 4f41db0 Compare March 22, 2024 09:26
@mergify mergify bot merged commit 6c1ba64 into develop Mar 22, 2024
40 checks passed
@mergify mergify bot deleted the wip/sergeigarin/add-back-and-forth-navigation branch March 22, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@somebody1234 somebody1234 somebody1234 left review comments

@PabloBuchu PabloBuchu PabloBuchu approved these changes

@indiv0 indiv0 indiv0 approved these changes

Assignees

@MrFlashAccount MrFlashAccount

Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge g-dashboard x-new-feature Type: new feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MrFlashAccount @somebody1234 @PabloBuchu @indiv0