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

Improve variables tab integration #4638

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

Improve variables tab integration #4638

wants to merge 4 commits into from

Conversation

abdul99ahad
Copy link
Contributor

@abdul99ahad abdul99ahad commented Oct 28, 2024

Closes #4516

Proposed Changes

Variables Application Menu

Variables_Tab

Variables Tab Keyboard shortcut

(For Mac OS): Press Command + Shift + V

Variable Tab keyboard shortcut

Variables Bottom Tab

Variable bottom tab

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Oct 28, 2024
@nikku nikku mentioned this pull request Oct 28, 2024
4 tasks
@abdul99ahad abdul99ahad force-pushed the variables-tab branch 6 times, most recently from 7e162c8 to b3a4f84 Compare October 28, 2024 14:35
@philippfromme
Copy link
Contributor

We should call this Toggle variables tab

image

I'd also add Toggle problems tab now that we have the other entry.

@barmac
Copy link
Collaborator

barmac commented Oct 30, 2024

I'd suggest that we decide on another key for the shortcut. Ctrl/Cmd+V is the traditional paste shortcut, and in some editors a combination of Cmd+Shift+(Option)+V triggers "paste and match formatting". Why don't we use a letter without strong OS link, e.g. U or M.

Screen.Recording.2024-10-30.at.17.26.31.mov

@nikku
Copy link
Member

nikku commented Oct 31, 2024

I'd suggest that we decide on another key for the shortcut. Ctrl/Cmd+V is the traditional paste shortcut, and in some editors a combination of Cmd+Shift+(Option)+V triggers "past and match formatting".

I subscribe to this. CC @lmbateman.

@nikku nikku requested a review from lmbateman October 31, 2024 10:00
@nikku nikku changed the title Variables tab Improve variables tab integration Oct 31, 2024
@lmbateman
Copy link

I agree that we should stay away from Cmd-V variations. Looks like for many MacOS apps (but not the Desktop Modeler), Cmd-M minimizes the window. Cmd-U is a common shortcut for underline, but since that's a fairly specialized feature, I think Cmd-U will work, and Ctrl-U should be fine for Windows. (Should we add a Cmd-M shortcut?)

@barmac
Copy link
Collaborator

barmac commented Oct 31, 2024

BTW the recording I shared displays the vscode shortcuts. I think the Shift usage in that tool is wise as it allows to avoid conflicts. Learning from Cmd+P (print or toggle properties panel 🤡 ).

@philippfromme
Copy link
Contributor

MacOS tests are passing now. Will review.

@philippfromme
Copy link
Contributor

I'd imagine a toggle entry for the bottom panel (we could use Ctrl+B) and (optional) entries for the individual tabs.

image

@philippfromme
Copy link
Contributor

@abdul99ahad Could you adjust the PR according to my suggestion?

  • add Toggle bottom panel entry with Ctrl+B shortcut
  • remove Reset properties panel entry (was a workaround created at the time where we had issues with its size, no longer needed)
  • decide whether to add individual entries for each tab

@abdul99ahad
Copy link
Contributor Author

@abdul99ahad Could you adjust the PR according to my suggestion?

  • add Toggle bottom panel entry with Ctrl+B shortcut
  • remove Reset properties panel entry (was a workaround created at the time where we had issues with its size, no longer needed)
  • decide whether to add individual entries for each tab

Okay. Regarding point 3, I think for now, we can add variables tab as individual entry and see in future if users also want Output and Problems tab in application menu.

@nikku
Copy link
Member

nikku commented Nov 5, 2024

Regarding point 3, I think for now, we can add variables tab as individual entry and see in future if users also want Output and Problems tab in application menu.

I'd say: Don't add a toggle shortcut for variables panel for now, but in the future automatically open variables tab from various places. Much more important that this is properly wired, i.e. with the FEEL editors.

@philippfromme
Copy link
Contributor

Additionally, adding the individual entries including disabled states is a bit more work since opening a tab doesn't trigger a menu update at the moment. So I'd say let's keep it for later or just don't add them if no one ever asks.

});

// when
app.triggerAction('open-bottom-panel');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
app.triggerAction('open-bottom-panel');
app.triggerAction('toggle-bottom-panel');

Copy link
Member

Choose a reason for hiding this comment

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

Let's rename to toggle, as toggle is what it is.

});

// when
app.triggerAction('open-bottom-panel');
Copy link
Member

Choose a reason for hiding this comment

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

Let's rename to toggle, as toggle is what it is.

@@ -466,7 +466,8 @@ export class BpmnEditor extends CachedComponent {
setColor: !!selectionLength,
spaceTool: !inputActive,
undo: commandStack.canUndo(),
zoom: true
zoom: true,
bottomPanel: true
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this variable? Why do we explicitly need to configure it? I assume that the bottom panel is always available, one way or the other.

Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Let's restore the previous bottom panel state (if applicable), once I toggle it open (again). This is standard behavior, and ensures I can temporarily close things, re-open if needed, and end up in the previous setup.

Right now I always end up on the Output tab, which is not desirable.

capture wbkSyL_optimized

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Currently worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve variables integration into Desktop Modeler
5 participants