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

task: set active header for workflow nodes #1564

Merged
merged 5 commits into from
Jul 24, 2023

Conversation

blanchco
Copy link
Contributor

Description

  • Set an "active" state for workflow nodes when they are opened in the drilldown panel. The header will be green when the drilldown for the workflow node is opened

As per the designs:
image

Working demo:

Screen.Recording.2023-07-24.at.1.23.44.PM.mov

@blanchco blanchco self-assigned this Jul 24, 2023
@YohannParis
Copy link
Member

If you have multiple sidebar opened, when you click on the tab, does the relevant node header turns grees?

@mwdchang
Copy link
Member

This change looks fine but they feels a bit short sighted - we had intended to build Terarium with a multi-window paradigm (e.g. you can open calibrate and simulation simultaneously). So this singular activeWorkflowNode doesn't really work as it becomes ill-defined.

I feel like this is feature that is not compatible going forward and it requires more thoughts and design.

@blanchco
Copy link
Contributor Author

If you have multiple sidebar opened, when you click on the tab, does the relevant node header turns grees?

It appears that only one tab is opened at a time when you open a node in the drilldown panel as of now. It does not handle multiple tabs in the drilldown (as far as I can tell)

@YohannParis
Copy link
Member

This change looks fine but they feels a bit short sighted - we had intended to build Terarium with a multi-window paradigm (e.g. you can open calibrate and simulation simultaneously). So this singular activeWorkflowNode doesn't really work as it becomes ill-defined.

I feel like this is feature that is not compatible going forward and it requires more thoughts and design.

I mean, even with multiple window, this feature is just to highlight the node that is currently open on the sidebar, not open on another tab or window.

@mwdchang
Copy link
Member

I mean, even with multiple window, this feature is just to highlight the node that is currently open on the sidebar, not open on another tab or window.

Sorry I am using "window" in a very generic sense here, I just mean opening multiple nodes simultaneously within the same application instance. Why build some thing that has a likelihood of getting teared down next week without ever seeing the lights of day?

@blanchco
Copy link
Contributor Author

blanchco commented Jul 24, 2023

I mean, even with multiple window, this feature is just to highlight the node that is currently open on the sidebar, not open on another tab or window.

Sorry I am using "window" in a very generic sense here, I just mean opening multiple nodes simultaneously within the same application instance. Why build some thing that has a likelihood of getting teared down next week without ever seeing the lights of day?

I guess this is worth further discussion. If we are doing a complete redesign of how we open and edit drilldowns (i.e. the ability to work on them simultaneously) then I agree this probably isn't worth putting in if that work on deck. If by opening multiple nodes at once we mean that we are just adding tabs to the drilldown sidepanel, we could just extend this 'node header highlighting' functionality based on the active tab
Screenshot 2023-07-24 at 2 31 39 PM

@mwdchang
Copy link
Member

Okay, for the sake of not spending hours and hours debating this without a concrete plan, let's merge it and prepare to rip it out. Also please I do intend on revisiting the workflow event-bus and make it closer to communicating workflow changes and not application state changes (which node is active is a state of the application and not a state of the workflow).

@mwdchang mwdchang merged commit df1a9ed into main Jul 24, 2023
@mwdchang mwdchang deleted the task-set-active-header-for-workflow-nodes branch July 24, 2023 19:48
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