-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add custom layout #832
base: dev
Are you sure you want to change the base?
Add custom layout #832
Conversation
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 might investigate further tomorrow but I noticed the following:
Opening the editor for the first time in a world now leads to an Error because position (I do not remember on what, sorry) cannot be set to undefined.
Which makes sense, since extent may be set before the panels are initialized |
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.
Code is ok, but I think we may need to discuss what we expect a full sized editor to look like
Fixed that and another bug |
Before or after merge? |
Before |
Also scrolling is not possible in the complete preview, just in the left half |
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 feel that it should not be necessary, but maybe @Paula-Kli or @SilvanVerhoeven can confirm - are deserialization checks necessary for extent
on the InteractiveGraph
the ResizablePanel
or for _latestSubWindowRatio
?
I'd also be in favor of merging this after the necessary PR in lively.next
was merged, would that be ok? If you'd prefer help bringing the changes that are necessary for Robins approval on the way we can tackle it on Monday @SilvanVerhoeven!
if (this.interactive && | ||
this.interactive.sequences.includes(Sequence.getSequenceOfMorph(morph))) { |
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.
Is this an intended change or was it introduced by windows formatting complications?
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.
Doesn't Sequence.getSequenceOfMorph already check whether there is a Sequence in the interactive?
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.
@linusha This is intended
@Paula-Kli No, it only traverses the morph owner change until the first Sequence
We won't change something about the zooming, as there are dependencies with other branches |
@@ -227,6 +272,7 @@ export class InteractivesEditor extends QinoqMorph { | |||
|
|||
connect(this.interactive, 'remove', this, 'reset'); | |||
connect(this.interactive, '_length', this.ui.menuBar.ui.scrollPositionInput, 'max').update(this.interactive.length); | |||
// TODO: let this work with zoom |
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'd appreciate if this stays for now please.
extent: { | ||
set (extent) { | ||
this.setProperty('extent', extent); | ||
// if (this._deserializing) return; |
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.
Is this save to remove? If so, can it go for good?
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 this is the same as the check below I would appreciate if we use this instead
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.
It's not, can/should be removed
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 the difference?
editor.js
Outdated
|
||
this.ui.interactiveGraph.extent = pt( | ||
this.ui.interactiveGraph.width, | ||
topWindowHeight - this.ui.interactiveGraph.submorphs[0].height - |
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 does the submorphs[0] do? Aren't there two submorphs in the interactiveGraph
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.
When the deserializing discussion is resolved
2895573
to
9904797
Compare
Co-authored-by: T4rikA <[email protected]> Co-authored-by: linusha <[email protected]>
Co-authored-by: frcroth <[email protected]> Co-authored-by: Paula-Kli <[email protected]>
9904797
to
93ed418
Compare
Co-authored-by: frcroth <[email protected]> Co-authored-by: Paula-Kli <[email protected]>
I re-requested reviews from everybody again since there were so many changes in here that I do not think that these actually reflect the review state. Feel free to just re-approve if you think otherwise. Personally, I'll not add a new review before we are done here. |
this.tree.extent = adjustedExtent; | ||
} |
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.
Why do we set the extent of the tree to the same as the overall extent? Shouldn't the tree be smaller as the search field needs to be subbtracted
Attention
|
f1e3bbb
to
a435a70
Compare
Closes #626