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

Refactor modular pipeline expansions to instantly update UI #2225

Merged
merged 21 commits into from
Jan 23, 2025

Conversation

SajidAlamQB
Copy link
Contributor

Description

This PR updates the logic for determining which nodes are visible when toggling expandAllPipelines. Previously, modular pipeline expansions were applied once during initial start, requiring data reloads to see changes. Now, the visibility logic is integrated directly into disabled.js, allowing the UI to reflect state changes immediately.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

The expand/collapse doesn't work with the NodeList in the sidebar.

@SajidAlamQB
Copy link
Contributor Author

I noticed for the first time the toggle doesn't update the view instantly but subsequent presses do.

@rashidakanchwala
Copy link
Contributor

I noticed for the first time the toggle doesn't update the view instantly but subsequent presses do.

yeah i noticed that too.

@SajidAlamQB
Copy link
Contributor Author

I noticed for the first time the toggle doesn't update the view instantly but subsequent presses do.

Fixed the onToggleExpandPipelines was toggling wrong way around.

@SajidAlamQB SajidAlamQB requested a review from Huongg January 13, 2025 11:59
@rashidakanchwala rashidakanchwala self-requested a review January 17, 2025 10:36
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

I have left some minor comments. Around repeated code

@rashidakanchwala rashidakanchwala self-requested a review January 20, 2025 15:07
Copy link
Contributor

@Huongg Huongg left a comment

Choose a reason for hiding this comment

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

Hey @SajidAlamQB, thank you for the updates! I have two main comments:

  • Could we rename the reducers TOGGLE_EXPAND_ALL_PIPELINES and TOGGLE_ALL_MODULAR_PIPELINES_EXPANDED to better reflect their purpose?

  • Both TOGGLE_ALL_MODULAR_PIPELINES_EXPANDED and normalize share similar logic (I replicated the logic from normalize). Could this be moved to a helper function?

Signed-off-by: Sajid Alam <[email protected]>
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

Hi Sajid, left two minor comments. Once those are fixed, then it should be good to merge. I am approving it for now.

@ravi-kumar-pilla
Copy link
Contributor

Hi @SajidAlamQB , Thank you for the PR. The code looks fine but I do not have complete context on the issue and I am finding it difficult understanding the variable names and what they do. I left few comments to clarify and better QA this PR. Thank you

@rashidakanchwala
Copy link
Contributor

Hi @SajidAlamQB , Thank you for the PR. The code looks fine but I do not have complete context on the issue and I am finding it difficult understanding the variable names and what they do. I left few comments to clarify and better QA this PR. Thank you

@ravi-kumar-pilla , this PR is just a refactor, earlier 'Expand All Pipelines' did a page reload everytime it was clicked, now it just uses redux actions as page reload was a bit of an overkill.

Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
@ravi-kumar-pilla
Copy link
Contributor

@ravi-kumar-pilla , this PR is just a refactor, earlier 'Expand All Pipelines' did a page reload everytime it was clicked, now it just uses redux actions as page reload was a bit of an overkill.

For some reason, I do not see a page reload on Expand All Pipelines click. I do see a main api call but not an entire page reload. I think we have an entire page reload only for changes in the Settings panel.

@rashidakanchwala
Copy link
Contributor

rashidakanchwala commented Jan 22, 2025

Apologies, not page load, that was incorrect term. The 'expand pipelines' functionality would trigger the URL parameter, which in turn ran preparePipelineState and then the pipelines got expanded. However, this didn't work with VSCode; right Sajid? Now, this happens directly through a redux trigger

@ravi-kumar-pilla ravi-kumar-pilla self-requested a review January 22, 2025 16:47
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ... Thank you @SajidAlamQB . I am not sure if we want to track this change in the release note.

@SajidAlamQB
Copy link
Contributor Author

Apologies, not page load, that was incorrect term. The 'expand pipelines' functionality would trigger the URL parameter, which in turn ran preparePipelineState and then the pipelines got expanded. However, this didn't work with VSCode; right Sajid? Now, this happens directly through a redux trigger

Yes that's correct although even this refactor still hasn't fixed the issue on vscode extension which probably has a more underlying issue on the extension side.

Signed-off-by: Sajid Alam <[email protected]>
@SajidAlamQB SajidAlamQB merged commit 214ee8f into main Jan 23, 2025
12 checks passed
@SajidAlamQB SajidAlamQB deleted the introduce-manage-toolbarOptions branch January 23, 2025 09:48
@SajidAlamQB SajidAlamQB mentioned this pull request Jan 30, 2025
5 tasks
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.

4 participants