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

feat: added separate colors for sideInput and dynamic legend #1292

Merged
merged 6 commits into from
Oct 30, 2023

Conversation

veds-g
Copy link
Contributor

@veds-g veds-g commented Oct 30, 2023

  • Added separate color for each generator nodes as well as for the corresponding input to the vertices
image
  • Added dynamic legend (only shows the nodes present in graph)
image
  • Fix graph height to occupy 100% area, when summary is collapsed / uncollapsed
image image
  • Added tooltip for graph interaction buttons
image
  • Styled the collapsed summary on pipeline view and adjusted the play/pause buttons, error panel and graph interaction panel
image

Signed-off-by: veds-g <guptavedant2312@gmail.com>
Signed-off-by: veds-g <guptavedant2312@gmail.com>
Signed-off-by: veds-g <guptavedant2312@gmail.com>
@@ -301,6 +305,7 @@ export function SummaryPageLayout({
]);

const contentMargin = useMemo(() => {
if (collapsable) return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Confused about this change. We want "contentMargin" to be the number of px to push the main content down so the summary isn't overlaying it.

Wouldn't this always return 0 for the pipeline's usage, given it is "collapsable"?

Looking further into the changes, I see what you are doing. I think it would be best to have SummaryPageLayout still apply the marginTop as it did before. If you want it to not apply any margin when it collapses, just pass offsetOnCollapse as false.

Adding it to context to adjust your other items (buttons and etc.) is fine, but i think the main layout margin should be controlled here.

GeneratorColorContext
);

const getSideInputColor = (nodeName: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

useCallback needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just put outside of the component so it is not in render loop.

};
};

if (data?.type === "sideInput") {
const generatorImage = {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 can be moved outside of component (const at top of file)

}}
/>
);
})}
</div>
{data?.nodeInfo?.sideInputs?.map((_, idx) => {
{data?.nodeInfo?.sideInputs?.map((input, idx) => {
const inputImage = {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 outside of component

Signed-off-by: veds-g <guptavedant2312@gmail.com>
@bbehnke bbehnke merged commit 17b7b31 into numaproj:main Oct 30, 2023
18 checks passed
whynowy pushed a commit to whynowy/numaflow that referenced this pull request Nov 1, 2023
…j#1292)

Signed-off-by: veds-g <guptavedant2312@gmail.com>
Co-authored-by: Bradley Behnke <bradleybehnke@yahoo.com>
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.

None yet

2 participants