-
Notifications
You must be signed in to change notification settings - Fork 112
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
Feature/slicing pipeline run command #1996
Feature/slicing pipeline run command #1996
Conversation
const { outerWidth: screenWidth } = chartSize; | ||
const actionBarWidth = | ||
ref.current && ref.current.firstChild.getBoundingClientRect().width; | ||
|
||
const metaDataPanelWidth = displayMetadataPanel ? 600 : 400; | ||
const nodeListWidth = visibleSidebar ? 200 : 400; | ||
|
||
const transformX = | ||
screenWidth - nodeListWidth - metaDataPanelWidth - actionBarWidth / 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good spot, I'm meant to add the minimum value for transformX so it never gets hidden behind the nodelist bar. Updated the code now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Huongg on demo-slicing-pipeline-final
branch I tested locally I still see action bar overlap with metadata panel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @jitu5 yeah it's supposed to be like that. The metadata panel can be displayed on the top of the run command. The only condition is the number of selected and run command should always be visible, and not being covered by the node list on the left
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Huongg I am fine with it. It would have been nice to fit action bar within available space, keeping command box as variable width container, may be we can keep this as future work after MVP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah interesting idea, i think it would require further design thinking as in order to fix the whole action bar in the available space, we will need to reduce the width, in that case what the transition/animation would look like, etc
Agree that can be something to be considered in the future MVP
src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.js
Outdated
Show resolved
Hide resolved
src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.js
Outdated
Show resolved
Hide resolved
Hi @Huongg , Overall the PR looks great. However, I observed few issues -
The functionality looks great, but there are some bumpy transitions on my machine. Thank you |
hey can you attach the screen recording from your side please? Also i realised the action bar should slice from right to left, not left to right. So updated it now. Also i see what you mean on the big screen. Will need to add break point in there but generally it does calculate based on the screen size, but we need to handle it differently when its at the breakpoint |
Sure - The screen size here is 2560 * 1080 . Some [Nit] design comments from what I observed -
In the above gif, if you see there is some movement in the run-command which is not smooth (may be it appears that way because the metadata panel closes at the same time). I feel the snackbar design would reduce the amount of movement the run-command takes. Also, in the gif if you see the run-command appears with 1 node selected (I think it should only appear when 2 are selected). Thank you |
hey @ravi-kumar-pilla as discussed one friday, I've now improved the transition of the action bar, and also fix the bug that you found when the first selected node is not highlighted. Let me know if you can see these changes from your side? |
hey @stephkaiser I think I've addressed all of your comments apart from point 3, let's discuss which approach we should go with for this one 😄 |
src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.js
Outdated
Show resolved
Hide resolved
src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.js
Outdated
Show resolved
Hide resolved
Signed-off-by: Huong Nguyen <huongg1409@gmail>
Signed-off-by: Huong Nguyen <huongg1409@gmail>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @Huongg !! 💯
Description
Fixes #1975
User story: As a Kedro user, I want to view my sliced pipeline and its corresponding run command so that I can run this slice in the CLI for my Kedro project. I also want to be able to reset my sliced pipeline view so that I can go back to viewing the full Kedro pipeline
Development:
run-command-pr.mov
QA notes:
When testing on your machine locally, or through gitpod, please note that this PR only covers the functionalities below:
Figma: https://www.figma.com/design/3kSpvIO1veLKfy9qHxuXwF/Kedro-WIP?node-id=1469-56945&t=gAB4IcCLRWoEGtZQ-0