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

Fix/focus mode modular pipeline highlight #1107

Merged
merged 7 commits into from Oct 5, 2022

Conversation

cvetankanechevska
Copy link
Contributor

@cvetankanechevska cvetankanechevska commented Sep 30, 2022

Description

Resolves #921

Development notes

Added one more boolean property to createInitialPipelineState , to track when FocusModeIcon is hovered and create new action in the store for updating this value.

For the style I reused the class we already have for when node is clicked and all other nodes in the flowchart are pushed in the background with some opacity (class: pipeline-node--faded).

QA notes

You can open the Gitpod link, and check the hover effect on FocusModeIcon in the left side node list.

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

Signed-off-by: Cvetanka <cventanka_nechevska@external.mckinsey.com>
Signed-off-by: Cvetanka <cventanka_nechevska@external.mckinsey.com>
Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you!

Maybe just write a test or two covering this new addition?


/**
* Update the value of the of the currently-active hovered node from focus mode
* @param {string|null} nodeHovered The node's unique identifier
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
* @param {string|null} nodeHovered The node's unique identifier
* @param {string|null} hoveredFocusMode The node's unique identifier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check my last commit for this fix. Thanks

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.

thanks @cvetankanechevska -- look's great!!! :)

nit - we can look at adding unit tests to actions, reducers, node-list and flowchart.

Signed-off-by: Cvetanka <cventanka_nechevska@external.mckinsey.com>
Signed-off-by: Cvetanka <cventanka_nechevska@external.mckinsey.com>
Signed-off-by: Cvetanka <cventanka_nechevska@external.mckinsey.com>
@cvetankanechevska
Copy link
Contributor Author

Hey @tynandebold and @rashidakanchwala I've added tests for the action, reducer and flowchart. Not sure about the test in the node-list component, since that action is already covered with the action and reducer tests. If you have any suggestions please let me know.

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.

looks good to me, great job 😄

Signed-off-by: Cvetanka <cventanka_nechevska@external.mckinsey.com>
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.

LGTM! thanks @cvetankanechevska

RELEASE.md Outdated
@@ -20,6 +20,7 @@ Please follow the established format:
- Remove Tags field from dataset and parameter nodes when viewed in metadata panel. (#1100)
- Fix keyboard support for exporting experiment runs. (#1103)
- Fix `MemoryDataSet` not displaying on metadata panel. (#1113)
- Add hovering on the FocusModeIcon. (#1107)
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
- Add hovering on the FocusModeIcon. (#1107)
- Enhance display of the flowchart when hovering over the FocusMode icon. (#1107)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this, I will change it.

@Mackay031
Copy link

Did I just mark this as resolved? If so, I apologise.

Signed-off-by: Cvetanka <cventanka_nechevska@external.mckinsey.com>
@cvetankanechevska cvetankanechevska merged commit fe3137b into main Oct 5, 2022
@cvetankanechevska cvetankanechevska deleted the fix/focus-mode-modular-pipeline-highlight branch October 5, 2022 09:31
@tynandebold tynandebold mentioned this pull request Jan 18, 2023
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.

Focus-mode modular pipeline highlight missing
6 participants