Add conditional panel closing transition based on height #1965

eeliana · 2022-07-17T02:20:23Z

What is the purpose of this pull request?

Resolves #1836

Overview of changes:
Only activate closing transition for panels when the height is below a certain height.

Anything you'd like to highlight/discuss:
Only adjusted closing transitions, opening transition remains the same.
Fixed height is 300 for now, can be adjusted.

Testing instructions:
Create a test site and put chunkier placeholder text into the panel to see the difference in closing transitions.

Proposed commit message: (wrap lines at 72 characters)
Add conditional panel closing transition based on height

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

…han a fixed height Better readability when chunky viewports are not "dragged" up to panel's start.

tlylt · 2022-07-17T02:56:37Z

Hi @eeliana, thank you for contributing!

Wonder if it is possible for you to add the two cases you showed in the gif in https://github.com/MarkBind/markbind/tree/master/packages/cli/test/functional/test_site so that this behavior is properly noted down in our functional test suite?

tlylt · 2022-07-17T03:07:06Z

@ang-zeyu do you have any examples of disorienting panels when closing? Looking at some of the panels in
https://markbind.org/userGuide/deployingTheSite.html#generating-a-github-personal-access-token and I feel that the closing animation is not too bad even when the content is long.

ang-zeyu · 2022-07-17T04:27:20Z

@tlylt was trying out the panels in the 2103 pages https://nus-cs2103-ay2122s2.github.io/website/schedule/week3/topics.html, which can be really long. The problem is worse on slower + mobile devices as well where the animation gets dragged out beyond the specified duration / becomes really prominent.

Might just be me though (I'm not a huge fan of large animations in regular day-to-day things - e.g. mac's mission control), perhaps we can get @jonahtanjz's input too.

I should also add that its more so disorienting for me only when the scroll animation takes place (the top of the panel is above the viewport), combined with the panel's height transition.

If in agreement here perhaps, instead of just a fixed height, we can also add:

if top of panel beyond viewport (I think there's some existing checks for this too) && this.$refs.panel.scrollHeight > 300
  don't animate

ang-zeyu · 2022-07-17T04:32:26Z

packages/vue-components/src/panels/PanelBase.js

@@ -141,6 +141,9 @@ export default {
          Thus, we need to reset the maxHeight to its current height for collapse transition to work.
        */
        this.$refs.panel.style.maxHeight = `${this.$refs.panel.scrollHeight}px`;
+        if (this.$refs.panel.scrollHeight > 300) {
+          this.$refs.panel.style.transition = 'max-height 0s ease-in-out';


On that note, I noticed the jQuery scroll animation isn't disabled too, was thinking it should (sorry that the issue description wasn't clear enough!)

@tlylt @eeliana in case you missed this.

It's nowhere near as disorienting with the panel height transition removed, but I'm not entirely sure if this should be removed as well. (personally, still favouring removal as there is a slight time lag till the scroll takes place)

If it looks ok / better to you guys feel free to keep it :)

jonahtanjz · 2022-07-17T11:45:44Z

If in agreement here perhaps, instead of just a fixed height, we can also add:
if top of panel beyond viewport (I think there's some existing checks for this too) && this.$refs.panel.scrollHeight > 300
 don't animate

Agree with this. I think the panel transition is fine as long as the header is within the viewport. I feel that it would be nice to have some transition when the panel is closed rather than it closing abruptly.

The transition only becomes disorienting when the panel is really long and causes the page to scroll when it is collapsed:

CS2103_T.-.Week.3.-.Topics.-.Google.Chrome.2022-07-17.19-36-14.mp4

tlylt · 2022-07-17T14:24:43Z

Hi @eeliana, you may need to address the functional test failures (See this part https://github.com/MarkBind/markbind/runs/7374887371?check_suite_focus=true)

Read https://markbind-master.netlify.app/devguide/workflow#updating-and-writing-tests to find out how to update the test snapshot.

ong6 · 2022-07-17T19:15:43Z

Great work with this PR!

Just a tip for updating tests:

To update tests you will have to run npm run updatetest after adding the new tests.
Following this remove all the files which are not relevant to the changes (like picture files and files with no actual changes)
Run npm run test again to see if it works

eeliana · 2022-07-20T16:16:40Z

Great work with this PR!

Just a tip for updating tests:

To update tests you will have to run npm run updatetest after adding the new tests.

Following this remove all the files which are not relevant to the changes (like picture files and files with no actual changes)

Run npm run test again to see if it works

Hi, I have done this and I don't see the same issue after npm run test locally. I'm not too sure what the issue is here. Can I get some help?

tlylt · 2022-07-20T16:24:44Z

Great work with this PR!
Just a tip for updating tests:

To update tests you will have to run npm run updatetest after adding the new tests.

Following this remove all the files which are not relevant to the changes (like picture files and files with no actual changes)

Run npm run test again to see if it works

Hi, I have done this and I don't see the same issue after npm run test. I'm not too sure what the issue is here. Can I get some help?

i don’t think you have updated the snapshot, have you tried running npm run updatetest? What @ong6 was referring to is that you should git add only the relevant generated files, and discard the changes to image files (and any other files that at the moment are incorrectly flagged)

Also, you should be able to run “npm run test” locally to check if the tests are passing locally , if they aren’t then it’s unlikely that they will pass in CI

eeliana · 2022-07-20T16:30:02Z

Great work with this PR!
Just a tip for updating tests:

To update tests you will have to run npm run updatetest after adding the new tests.

Following this remove all the files which are not relevant to the changes (like picture files and files with no actual changes)

Run npm run test again to see if it works

Hi, I have done this and I don't see the same issue after npm run test. I'm not too sure what the issue is here. Can I get some help?

i don’t think you have updated the snapshot, have you tried running npm run updatetest? What @ong6 was referring to is that you should git add only the relevant generated files, and discard the changes to image files (and any other files that at the moment are incorrectly flagged)

Also, you should be able to run “npm run test” locally to check if the tests are passing locally , if they aren’t then it’s unlikely that they will pass in CI

Yup, I ran npm run updatetest before npm run test. Is the issue because I did not commit the generated files? I can't seem to reproduce the Unequal number of files error locally although the check is failing because of it.

tlylt · 2022-07-20T16:35:50Z

Great work with this PR!
Just a tip for updating tests:

To update tests you will have to run npm run updatetest after adding the new tests.

Following this remove all the files which are not relevant to the changes (like picture files and files with no actual changes)

Run npm run test again to see if it works

Hi, I have done this and I don't see the same issue after npm run test. I'm not too sure what the issue is here. Can I get some help?

i don’t think you have updated the snapshot, have you tried running npm run updatetest? What @ong6 was referring to is that you should git add only the relevant generated files, and discard the changes to image files (and any other files that at the moment are incorrectly flagged)
Also, you should be able to run “npm run test” locally to check if the tests are passing locally , if they aren’t then it’s unlikely that they will pass in CI

Yup, I ran npm run updatetest before npm run test. Is the issue because I did not commit the generated files? I can't seem to reproduce the Unequal number of files error locally although the check is failing because of it.

Yes you will need to commit the generated files, they are generally in packages/cli/test/functional/test_site/expected

take note of the caveat in #1937

tlylt · 2022-07-22T13:13:48Z

@ang-zeyu I am not sure if the disorienting issue can be resolved even by removing the transition effect. Trying it out locally with the changes from this branch:

For pages like the one below, with a panel that is not too long, I prefer the previous closing animation since it looks fine to me. With the new changes, the closing is more abrupt and it may be a little surprising if another shorter panel nearby has an animation and suddenly the next panel doesn't have it

What do you think?

ang-zeyu · 2022-07-22T14:21:18Z

packages/vue-components/src/panels/PanelBase.js

@@ -141,6 +141,9 @@ export default {
          Thus, we need to reset the maxHeight to its current height for collapse transition to work.
        */
        this.$refs.panel.style.maxHeight = `${this.$refs.panel.scrollHeight}px`;
+        if (this.$refs.panel.scrollHeight > window.innerHeight) {


@ang-zeyu I am not sure if the disorienting issue can be resolved even by removing the transition effect. Trying it out locally with the changes from this branch:

I don't think this implementation is correct, @tlylt the case in your gif shouldn't occur ⬇️

if top of panel beyond viewport (I think there's some existing checks for this too)

the relevant existing check is here https://github.com/MarkBind/markbind/blob/master/packages/vue-components/src/panels/PanelBase.js#L147

For pages like the one below, with a panel that is not too long, I prefer the previous closing animation since it looks fine to me. With the new changes, the closing is more abrupt and it may be a little surprising if another shorter panel nearby has an animation and suddenly the next panel doesn't have it

If corrected, does it account for the 2 links you linked too?

If not, we could try #1965 (comment), or even remove the closing transitions completely for UX consistency (⭐I think this might be better based on what you've mentioned⭐).

Again this is all very subjective, and I'm not huge fan of large animations (fwiw I'm the original implementor of this feature, haven't found the closing transitions anything apart from dizzying)
Our primary issue back then was also that the page did not scroll back up to the top of the panel at all, much less the transitions which was a cool "afterthought" =P

If agreed here perhaps we should just remove closing transitions completely (including the jquery scroll).
@damithc @MarkBind/active-devs can chime in too.

maybe give a thumbs up 👍 or 👎 on this comment to get a consensus. :-)

I'm OK to remove the transition.

@eeliana based on the 👍 so far, I think we can go ahead with replacing any and all closing transitions (including the jquery scroll) with an "instant snap" back to the top of the panel. The opening ones can be kept.

@ang-zeyu

If corrected, does it account for the 2 links you linked too?

Yes if all closing transitions are removed, then I think the behavior is uniform and I am ok with that.

I don't think this implementation is correct, @tlylt the case in your gif shouldn't occur ⬇️

Re looking at this again, may need your advice on if there's anything special going on here... So the case in my gif is one that the closed panels are near the bottom of the page. When opening or closing, the height of the page could reduce or increase such that the panel moves away from the original position. Base on my observation, whether before or after the latest change done by @eeliana, it seems like at times the panel could move away from the original position and result in slight disorienting effect, or somehow it won't. Here's a few recordings:

You can see that the first closing of "Guideline: Maximum readability" result in the panel heading moving, while the open and closing of the subsequent times didn't cause that (which is what we want to achieve?). This is done with the change in this PR applied

You can also experiment with this on https://nus-cs2103-ay2021s1.github.io/website/se-book-adapted/chapters/codeQuality.html which is the original behavior:
a) works fine (seems like I am able to trigger this, by closing the panels one by one, avoiding any scrolling before I close all the panels)

b) doesn't really work well (seems like I am able to trigger this, by scrolling around, before closing the panels)

@eeliana Please feel free to experiment a little and follow up on this since it's your PR :)

Re looking at this again, may need your advice on if there's anything special going on here...

I actually thought the issue you wanted to show in your gif was that the closing transition was not activating previously, in that case it should since the top of the panel is in view. (the check isn't correct for that)

Yes if all closing transitions are removed, then I think the behavior is uniform and I am ok with that.

But not so much relevant since we're going with this now. 😶

Re looking at this again, may need your advice on if there's anything special going on here...

This seems separate, let's put up a another issue for this.

Since its occuring only on the first open, likely:

there is some dynamically loaded content (e.g. <panel src="...">) inside that panel

something to do with how we handle these updates (see here and here) and/or how chrome handles these viewport changes (on that note, maybe we could try different browsers too)

packages/cli/test/functional/test_site/testPanelsClosingTransition.md

ang-zeyu · 2022-07-31T04:56:48Z

packages/vue-components/src/panels/PanelBase.js

-          // To enable behaviour of auto window scrolling during panel collapse
-          if (this.$el.getBoundingClientRect().top < 0) {
-            const headerHeight = jQuery('header[sticky]').height() || 0;
-            jQuery('html').animate({


let's update this with an instant scroll; this was the key issue we had back then. Again the transitions are just a "cool afterthought" =P

ang-zeyu

the logic here needs to be updated too https://github.com/MarkBind/markbind/blob/master/packages/vue-components/src/panels/MinimalPanel.vue#L123 (try collapsing a minimal type panel, the header would disappear)

ang-zeyu · 2022-07-31T05:06:25Z

packages/vue-components/src/panels/PanelBase.js

      } else {
        // Expand panel
        this.$refs.panel.style.maxHeight = `${this.$refs.panel.scrollHeight}px`;
+        this.$refs.panel.style.transition = 'max-height 0.5s ease-in-out';


👀 could this go before the previous line? I don't think there's any browser guarantees here but it would be best to ensure the sequence of operations on our side too.

ang-zeyu · 2022-08-13T05:40:42Z

packages/vue-components/src/panels/PanelBase.js

        requestAnimationFrame(() => {
          // To enable behaviour of auto window scrolling during panel collapse
          if (this.$el.getBoundingClientRect().top < 0) {
            const headerHeight = jQuery('header[sticky]').height() || 0;
            jQuery('html').animate({
              scrollTop: window.scrollY + this.$el.getBoundingClientRect().top - headerHeight - 3,
-            }, 500, 'swing');
+            }, 0, 'swing');


Why not remove the animate?

.scrollTop = ...

tlylt · 2023-02-05T02:04:41Z

Hi @eeliana, great work on this PR. Checking if you are still keen to implement the last few comments/fixes as requested?

tlylt · 2023-02-12T02:37:48Z

cc @MarkBind/contributors pls help support this if possible, it's near completion.

yucheng11122017 · 2023-02-12T06:25:44Z

@tlylt I don't seem to have permission to edit the PR.

tlylt · 2023-02-12T06:38:18Z

@tlylt I don't seem to have permission to edit the PR.

Pls checkout from this branch and raise a follow-up PR instead.

tlylt · 2023-02-28T01:45:48Z

Closing after work is merged in #2159, thank you @eeliana and @yucheng11122017!

eeliana added 2 commits July 16, 2022 21:33

Update panel transition to activate only if panel's content is less t…

fc26ec3

…han a fixed height Better readability when chunky viewports are not "dragged" up to panel's start.

Replace double quotes with single quotes

5304b50

Add panels closing behaviours in test site

d3c5cbc

ang-zeyu reviewed Jul 17, 2022

View reviewed changes

Update closing animation condition

d6a80e4

eeliana marked this pull request as draft July 17, 2022 14:27

Update tests for panels closing transition

bbe4e62

Remove include from index.md

b9a71bb

Add expected test files

b224143

eeliana marked this pull request as ready for review July 20, 2022 16:43

tlylt requested a review from ong6 July 22, 2022 13:14

ang-zeyu reviewed Jul 22, 2022

View reviewed changes

Remove all panel closing transitions

37852a6

tlylt requested changes Jul 28, 2022

View reviewed changes

packages/cli/test/functional/test_site/testPanelsClosingTransition.md Outdated Show resolved Hide resolved

Update tests for removal of panels closing transition

2d8f9b3

ang-zeyu reviewed Jul 31, 2022

View reviewed changes

Replace animation with instant scroll

6128b01

ang-zeyu reviewed Aug 13, 2022

View reviewed changes

yucheng11122017 mentioned this pull request Feb 12, 2023

Remove panel closing transition #2159

Merged

11 tasks

tlylt closed this Feb 28, 2023

tlylt mentioned this pull request Jul 17, 2023

Closing a panel without animation is disorientating #2337

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conditional panel closing transition based on height #1965

Add conditional panel closing transition based on height #1965

eeliana commented Jul 17, 2022

tlylt commented Jul 17, 2022

tlylt commented Jul 17, 2022

ang-zeyu commented Jul 17, 2022

ang-zeyu Jul 17, 2022

ang-zeyu Jul 21, 2022

jonahtanjz commented Jul 17, 2022

tlylt commented Jul 17, 2022

ong6 commented Jul 17, 2022 •

edited

Loading

eeliana commented Jul 20, 2022 •

edited

Loading

tlylt commented Jul 20, 2022 •

edited

Loading

eeliana commented Jul 20, 2022

tlylt commented Jul 20, 2022

tlylt commented Jul 22, 2022

ang-zeyu Jul 22, 2022

ang-zeyu Jul 22, 2022 •

edited

Loading

damithc Jul 22, 2022

ang-zeyu Jul 25, 2022

tlylt Jul 30, 2022

ang-zeyu Jul 31, 2022

ang-zeyu Jul 31, 2022

ang-zeyu left a comment

ang-zeyu Jul 31, 2022

ang-zeyu Aug 13, 2022

tlylt commented Feb 5, 2023

tlylt commented Feb 12, 2023

yucheng11122017 commented Feb 12, 2023

tlylt commented Feb 12, 2023

tlylt commented Feb 28, 2023

Add conditional panel closing transition based on height #1965

Add conditional panel closing transition based on height #1965

Conversation

eeliana commented Jul 17, 2022

tlylt commented Jul 17, 2022

tlylt commented Jul 17, 2022

ang-zeyu commented Jul 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahtanjz commented Jul 17, 2022

tlylt commented Jul 17, 2022

ong6 commented Jul 17, 2022 • edited Loading

eeliana commented Jul 20, 2022 • edited Loading

tlylt commented Jul 20, 2022 • edited Loading

eeliana commented Jul 20, 2022

tlylt commented Jul 20, 2022

tlylt commented Jul 22, 2022

Choose a reason for hiding this comment

ang-zeyu Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ang-zeyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlylt commented Feb 5, 2023

tlylt commented Feb 12, 2023

yucheng11122017 commented Feb 12, 2023

tlylt commented Feb 12, 2023

tlylt commented Feb 28, 2023

ong6 commented Jul 17, 2022 •

edited

Loading

eeliana commented Jul 20, 2022 •

edited

Loading

tlylt commented Jul 20, 2022 •

edited

Loading

ang-zeyu Jul 22, 2022 •

edited

Loading