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

Variables-specific keyboard commands #14165

Merged
merged 2 commits into from
Aug 17, 2022
Merged

Variables-specific keyboard commands #14165

merged 2 commits into from
Aug 17, 2022

Conversation

philrenaud
Copy link
Contributor

@philrenaud philrenaud commented Aug 17, 2022

Adds variables-specific keyboard nav:

  • g v to go to variables, globally
  • v to show/hide values on a variable page
  • j to switch to/from JSON mode when viewing or editing a variable
  • Shift + NUM to enter a folder/file

image

( Port of 68e5999 and 357b3d7 )

Copy link
Contributor

@ChaiWithJai ChaiWithJai left a comment

Choose a reason for hiding this comment

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

Hey buddy, great job! But, I think this should will into errors with namespaces.

<td colspan="3"
{{keyboard-shortcut
enumerated=true
action=(fn this.handleFolderClick folder.data.absolutePath)
Copy link
Contributor

Choose a reason for hiding this comment

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

blocking: This should accept a namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clicking into a folder doesn't do any variable read, only variable list (like clicking into a folder in a file directory). As such, if the folder is present at all it means the user has access to list at least one variable within it and it should remain clickable. Thus, no namespace passing.

In addition: the concept of "folders" are generated in the ui and do not themselves even have namespaces. They can contain many variables from many different namespaces. So nothing to pass in even if it was a good idea to do so.

@@ -29,6 +34,10 @@
data-test-file-row
{{on "click" (fn this.handleFileClick file)}}
class={{if (can "read variable" path=file.absoluteFilePath namespace=file.variable.namespace) "" "inaccessible"}}
{{keyboard-shortcut
enumerated=true
action=(fn this.handleFileClick file)
Copy link
Contributor

Choose a reason for hiding this comment

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

blocking: This should accept a namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ChaiWithJai — the handleFileClick action reads namespace from the variable (passed here as file) itself. As such , namespaces do not need to be passed in. This is the same logic we use a couple lines above this in the on "click".

@philrenaud philrenaud merged commit fd148aa into main Aug 17, 2022
@philrenaud philrenaud deleted the keynav-variables branch August 17, 2022 18:44
@github-actions
Copy link

github-actions bot commented Aug 17, 2022

Ember Asset Size action

As of d37540d

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +780 B +139 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

Ember Test Audit comparison

main d37540d change
passes 1410 1410 0
failures 0 0 0
flaky 0 0 0
duration 9m 48s 628ms 10m 22s 658ms +34s 030ms

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2022
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.

2 participants