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 Resize Right Flyout using drag #2224

Open
wants to merge 28 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
Copy link
Contributor Author

Old PR: #2138

@srikant-ch5
Copy link
Contributor Author

Hi @matthoward366 Is it ok to include properties-right-flyout-container the way I included it in this PR or will it break any functionality in the consuming applications ?
I included it to align the resize button so that content spreads out in the properties-panel.
( I will include comments and will refactor the code in following commits)

@matthoward366
Copy link
Member

@nmgokhale can you have a look?

@srikant-ch5 srikant-ch5 marked this pull request as ready for review October 25, 2024 14:54
Signed-off-by: srikant <[email protected]>
@srikant-ch5
Copy link
Contributor Author

@tomlyn Could you please let me know whether cc-rightflyout code is ok ? I have tried to keep it just like cc-bottompanel with few changes like using ref to limit panel to 70%.

@@ -138,7 +138,7 @@ class CommonCanvasPanels extends React.Component {
containingDivId={this.props.containingDivId}
/>
);
const rightFlyout = (<CommonCanvasRightFlyout />);
const rightFlyout = (<CommonCanvasRightFlyout containingDivId={this.props.containingDivId} canvasController={this.props.canvasController} />);
Copy link
Member

Choose a reason for hiding this comment

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

Can't this code be structured the same way as we do it for left flyout below? That is, package it in a function; return null if the panel is not open etc. I'd like the code that handles the left flyout and the code that handles the right flyout be as much as possible structured in the same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to include isRightPanelOpen also updated common-canvas-test.js as the condition { showRightFlyout: false } was failing because of the change.

setRightPanelWidth(wdth) {
this.store.dispatch({ type: "SET_RIGHT_FLYOUT_CONFIG", data: { config: { panelWidth: wdth } } });
}

Copy link
Member

Choose a reason for hiding this comment

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

If you look at the comments this is in a section for 'Bottom panel methods'. There is a section above for 'Right flyout methods' where this code should go.

Also in the 'Right flyout methods' can we add a isRightFlyoutOpen() method to keep things consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated as per above comment.

@tomlyn
Copy link
Member

tomlyn commented Oct 25, 2024

Well if it works I suppose it's OK but it isn't the same as the way bottom panel is structured which is what I was looking for or -- as I said previously -- if the way you have written the new code is somehow better than the way bottom panel is done then we should discuss making bottom panel work the same way. I probably haven't looked at it closely enough but I'm not sure why you are using the ref

Also, if we are going to use refs, I thought the appropriate way to create them is to use the createRef() function
https://react.dev/reference/react/createRef At least that is how I have created them in other class components that we have in Common Canvas.

@tomlyn
Copy link
Member

tomlyn commented Oct 25, 2024

Also, don't forget we will need a few Cypress tests for this new function.

@srikant-ch5
Copy link
Contributor Author

srikant-ch5 commented Nov 7, 2024

Well if it works I suppose it's OK but it isn't the same as the way bottom panel is structured which is what I was looking for or -- as I said previously -- if the way you have written the new code is somehow better than the way bottom panel is done then we should discuss making bottom panel work the same way. I probably haven't looked at it closely enough but I'm not sure why you are using the ref

I have updated the code to be pretty much like bottom-panel apart from the limitWidth function because as per requirement panel should should extend upto 70% leaving 30% width for canvas. Unlike bottom-panel right-panel is a part of a grid.

Also, if we are going to use refs, I thought the appropriate way to create them is to use the createRef() function https://react.dev/reference/react/createRef At least that is how I have created them in other class components that we have in Common Canvas.

I have removed refs. Thanks.

Also, don't forget we will need a few Cypress tests for this new function.

I have included some Cypress Tests for the new function. In the build I can see the few Cypress Tests are failing which when I test in my local (even in local prod env) they are passing, so will look into this issue.

Copy link
Member

@tomlyn tomlyn left a comment

Choose a reason for hiding this comment

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

@srikant-ch5 Looks good so far. I added a few comments.

docs/pages/03.02.01-canvas-config.md Show resolved Hide resolved
@@ -420,11 +427,19 @@ class PropertiesMain extends React.Component {
this.setState({ showPropertiesButtons: state });
}

updateRightFlyoutWidth(size) {
const element = document.querySelector(".common-canvas .right-flyout-container");
Copy link
Member

Choose a reason for hiding this comment

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

This is creating a dependency from Common Properties on Common Canvas. You could discuss with @matthoward366 @nmgokhale and @caritaou but I don't think we should be doing that. Common Properties and Common Canvas should remained decoupled, so each can always be used independently of the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nmgokhale Would appreciate your feedback on above I can see similar common properties dependancy on common canvas over here

Without enforcing width to right-flyout in properties it will look something like below, where the properties content width is set as per custom classnames but flyout width is not reset.

Screenshot 2024-11-12 at 10 52 49 PM

background: $layer-01;
transition: all 0.2s ease-in;
&:hover {
flex: 0 0 $spacing-01;
Copy link
Member

Choose a reason for hiding this comment

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

What is this for - is it necessary? I took it out and the right flyout seems to size OK. If this is needed to get Common Properties to display correctly? If so it ought to be part of the Common Properties CSS, not here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated above to include spacing-01 in right-flyout-drag.

But the problem with above spacing is that when a common properties Accordion is highlighted on click there is a small gap between right flyout left border and common properties.
Screenshot 2024-11-12 at 10 15 53 PM

I tried to resolve this using common properties css but this is visible in canvas so initially I set the width to 0.5px so that the gap isn't visible and on hover it will take up 1px width to match with bottom panel, but I have reverted it now and kept spacing-01 for the right-flyout-drag.

Screen.Recording.2024-11-12.at.10.42.51.PM.mov

Copy link
Member

@tomlyn tomlyn Nov 12, 2024

Choose a reason for hiding this comment

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

If you are concerned about the visual appearance of the UI after you've implemented a change you should be asking @Analuisa-Del-Rivero1 for her decision on how she wants it to appear. Unfortunately your video doesn't show what it looks like with the accordion highlighting.

Copy link
Member

Choose a reason for hiding this comment

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

BTW It's probably best to ask question to Ana in the issue to keep all the design information discussion together in one place.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you could do a video (or two?) that shows both approaches and what the accordion looks like in each case and explain to her about the difficulty in hovering over the drag area ... and see what she decides.

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
4 participants