-
Notifications
You must be signed in to change notification settings - Fork 123
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
Rewrite panel transition code #1430
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.
Nice work with the rewrite! @wxwxwxwx9
Left some observations that may require some reworking internally; also, expanded
+ src
attribute currently leaves the panel 'stuck'
Since there are quite a few combinations of options which affect the loading, you may want to try creating a whole bunch of <panel>
s mixed with / without these options which may affect the functionality:
- vanilla
preload
expanded
src
type=minimal
minimized
Hi @ang-zeyu , thank you so much for your review and suggestions. I think with this new commit, most (if not all) of the functionalities should be working properly. I have tested with all the options you listed. There doesn't seem to be any regressions (fingers crossed) based on the exploratory testing that I've done. There is, however, one "bug" in my code that I have resolved but don't quite seem to understand the root cause. I was thinking maybe you would know something about it. I have documented my observations about that particular section thoroughly. It's regarding the nextTick issue :-/ I have ensured that Hopefully there won't be anymore major issues like the src/retriever issue you pointed out! |
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.
Nice work again.
One minor bug left, and couple of nits:
Let's standardise to use /* ... */
for multiline comments too
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.
one more edge case and some nits:
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.
Lgtm 👍
Let's updatetest
7c07a3c
to
3bdfa33
Compare
What is the purpose of this pull request?
This PR is meant to rewrite some of the panel transition code and lay the groundwork to implement a cleaner version of the panel preview feature (related to #1425).
Overview of changes:
Removed the usage of v-show for the panel refs for both nestedPanel and minimalPanel. Implemented new transition code with javascript and CSS (max-height).
Anything you'd like to highlight / discuss:
I tried to ensure that there is no regression to the existing functionalities for my rewrite. There were some code that I have removed, like the event @src-loaded and setMaxHeight from the retriever, as I did not see the need for it for my implementation. However, I'm unsure if I am right in doing that or whether it would cause regressions in ways that I couldn't foresee. Do let me know if I am breaking any existing features by doing that :O
Also, perhaps my code quality can be improved (e.g. the way I implement certain functionalities). So, I think that may be one area to look at.
I have written some comments in my code to explain the rationale for some of my implementations.
Testing instructions:
markbind serve
and test the panels on the user guide section.Proposed commit message: (wrap lines at 72 characters)
Rewrite panel transition for preview feature
It is not easy to implement the panel preview feature with the existing
transition code in our panels due to the use of v-show.
Let's rewrite some of the transition code and implement our own set
of transition code to allow for more flexibility. This will allow a cleaner
implementation of the panel preview feature.
Checklist: ☑️