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

#2137 Enable common properties right flyout resize using drag #2138

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

srikant-ch5
Copy link
Contributor

Fixes: #2137

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@srikant-ch5 srikant-ch5 marked this pull request as draft September 3, 2024 11:14
@srikant-ch5
Copy link
Contributor Author

Hi @matthoward366 I have removed test cases related to resize buttons as they are replaced with drag, is this okay ?

Screen.Recording.2024-09-03.at.5.38.15.PM.mov

@matthoward366
Copy link
Member

The design expected to keep the buttons as well as having the dragging option. That way customer can quickly increase the size.

This also needs to be behind a feature flag since not all customers would want this.

@srikant-ch5 srikant-ch5 changed the title #2137 Increase View Size of Modeler Node Panel #2137 Enable common properties right flyout resize using drag Sep 3, 2024
@tomlyn
Copy link
Member

tomlyn commented Sep 3, 2024

@matthoward366

The design expected to keep the buttons as well as having the dragging option.

I think Ana suggests this behavior would replace the buttons. Which is why, I assume @srikant-ch5 has done it this way. I've queried that with Ana

@srikant-ch5 I think we ought to implement this is a different way. Right now the user can dynamically size the top panel and bottom panel by drag/drop and that code is in Common Canvas. I think it makes sense therefore that we should have similar code for the right-flyout (and also probably the left flyout too) inside Common Canvas - not in Common Properties. That way applications can get the sizing behavior even if they are not using Common-Properties. As Matt points out, not all applications will want to the drag/drop sizing behavior for the right-flyout so we should have a feature flag for that (an one for the left flyout too if we implement drag/drop sizing for that too).

@srikant-ch5
Copy link
Contributor Author

Hi @tomlyn Yes correct, I checked with her regarding few other queries.

Sure I will revert the changes in Common Properties and will try to implement this in Common Canvas.

Signed-off-by: srikant <[email protected]>
@srikant-ch5
Copy link
Contributor Author

@tomlyn I Included the sizing behaviour in Common Properties because I tried implementing it CommonCanvas Right flyout and when I expand the panel content dosen't grow/shrink with it, will try to look for a solution for this.

Screen.Recording.2024-09-04.at.10.58.51.AM.mov

@matthoward366
Copy link
Member

@srikant-ch5 can you look at the conflict and build failure?

@srikant-ch5
Copy link
Contributor Author

srikant-ch5 commented Sep 12, 2024

Hi @matthoward366 is it okay if we set the width of properties right flyout like below in properties-main.jsx since we are implementing this in common canvas and the properties width should also be applied on cc right lfyout.

and in cc-right-flyout like below

righFlyoutChildNode.classList.remove("properties-small", "properties-medium", "properties-large", "properties-max");

I haven't changed anything related to panel sizes in properties-main.scss

Currently it behaves like below to enable extend upto 70% of available width and minimum width. Please let me know your thoughts on this or if you want to suggest any other approach to this. Above changes are in PR.

Screen.Recording.2024-09-12.at.10.51.13.PM.mov

@@ -87,6 +87,12 @@ class PropertiesMain extends React.Component {
showPropertiesButtons: true,
editorSize: editorSize
};
this.flyoutWidth = {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing this being used. Am I missing something?

@@ -419,11 +426,19 @@ class PropertiesMain extends React.Component {
this.setState({ showPropertiesButtons: state });
}

updateRightFlyoutWidth(size) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems like the wrong place for setting this. Why is this needed here?

@@ -58,6 +59,7 @@ $properties-resize-button-size: $spacing-06;
padding: $spacing-01;
justify-content: center;
align-items: center;
z-index: 11;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? This might cause other issues.


// In case of Common Properties remove hardcoded width to allow content to take full width.
const righFlyoutChildNode = this.commonCanvasRightFlyout.childNodes[1];
if (righFlyoutChildNode && righFlyoutChildNode.className.includes("properties-right-flyout")) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems wrong since it's mixing common properties and common-canvas.

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, removed this code from CC.

@@ -35,7 +84,12 @@ class CommonCanvasRightFlyout extends React.Component {
? "right-flyout-panel under-toolbar"
: "right-flyout-panel";
rightFlyout = (
<div className={rfClass}>
<div className={rfClass} id="right-flyout-panel" ref={ (ref) => (this.commonCanvasRightFlyout = ref) }>
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't have an id of right-flyout-panel. We should try not to use ids in common-canvas or common-properties. If we do, then they need to be unique for that canvas since we can have multiple instances in the DOM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have removed this now.

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.

Enable common properties right flyout resize using drag
3 participants