Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
#2137 Resize Right Flyout using drag #2224
Changes from 22 commits
24fc35e
4672b3d
b960ffe
c186d1a
26c4ca5
c3fce9e
56c93d1
9a0e490
7ca8ccd
004c49f
0071ea1
216f59d
2da678d
ba94e30
09bb046
7b16e8b
e757c09
c381e6c
d92e6db
f31a076
eea68de
7268860
60a9f5d
e4b80c6
6336f85
13024aa
536d6fc
4dcabc9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
I have updated above to include
spacing-01
inright-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.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 up1px
width to match with bottom panel, but I have reverted it now and keptspacing-01
for theright-flyout-drag
.Screen.Recording.2024-11-12.at.10.42.51.PM.mov
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.
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.
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.
BTW It's probably best to ask question to Ana in the issue to keep all the design information discussion together in one place.
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.
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.
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.
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.
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.
@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
inproperties
it will look something like below, where the properties content width is set as per custom classnames but flyout width is not reset.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.
Before this PR we didn't have a default width -- right? So the behavior was that the right-flyout panel would size to fit the content of the JSX the app placed into it. I don't think we can change that behavior because we will upset existing applications that are using the right-flyout.
For example, the
Flows
sample application has a behavior where right-flyout is opened when the user double clicks on a node. However with your PR the right-flyout is visible (but empty) with its default width even before the user double clicks a node. Like this:I believe this is because Common Properties is deciding whether it is open or closed. When closed it is returning an empty (null) JSX. Previously, this would have caused Common Canvas to add an empty
<div>
for the right-flyout to the DOM however your PR is now adding the<div>
which is empty but with a default width of 300.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.
I have updated this to
rightflyout: { panelWidth: 0 }
would that be ok because incc-right-flyout
it is causing issue without default width set in store.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.
The current right-flyout behavior allows an application to add some content into the right-flyout and it stretches to accommodate that content. (Remember applications can put whatever content they like into the right-flyout. They don't necessarily have to put Common Properties in there.)
So we need to make sure that function still works. Does that still work if we set the size to 0?