Replace jQuery animation for table of contents and remove jQuery package #771

swarajpure · 2021-07-31T21:29:15Z

Fixes #769

swarajpure · 2021-07-31T21:50:56Z

tests/integration/components/table-of-contents-test.js

+    assert.dom('ol.toc-level-1').isNotVisible;
+    assert.dom('.toc-level-1').doesNotHaveClass('selected');
+
+    await click(document.querySelector('.toc-level-0 > a'));


The button is being referred thrice here, will make it into a variable rather than repeating the code

package.json

app/templates/components/table-of-contents.hbs

tests/integration/components/table-of-contents-test.js

ankushdharkar · 2021-08-01T23:14:38Z

app/styles/components/_sidebar.scss

@@ -121,11 +121,17 @@ li.toc-level-0 {
 ol.toc-level-1 {
  border-left: $base-border;
  font-size: $base-font-size * 0.9;
+  transition: max-height 0.5s ease;


Should we be animating height or max-height?

We can't actually use height. For the expanded condition, we would have to give height: auto and According to the Mozilla Developer Network docs, auto values have been intentionally excluded from the CSS transitions spec.
Source: https://css-tricks.com/using-css-transitions-auto-dimensions/

ankushdharkar

Great! Love that you added new tests. Some small queries in the comments.

jenweber · 2021-08-05T12:22:15Z

Hi! Thanks for your work on this! I agree with Ankush that isVisible should be isVisible() instead. Once that is fixed, this will be ready to merge. I tested this locally and it worked great.

swarajpure · 2021-08-06T20:22:30Z

Hi! Thanks for your work on this! I agree with Ankush that isVisible should be isVisible() instead. Once that is fixed, this will be ready to merge. I tested this locally and it worked great.

Thanks!
Done with the changes.

Can you please review now @jenweber @ankushdharkar? 😄

jenweber · 2021-08-08T20:42:51Z

Hi @swarajpure, thanks for your work on this! I see the following failing in CI. I think it's unrelated to your PR and I have opened a separate PR to try and fix it. #772

yarn install v1.22.11
[1/5] Validating package.json...
error [email protected]: The engine "yarn" is incompatible with this module. Expected version "1.22.10". Got "1.22.11"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
Error: Process completed with exit code 1.

swarajpure · 2021-08-09T19:55:11Z

Hi @swarajpure, thanks for your work on this! I see the following failing in CI. I think it's unrelated to your PR and I have opened a separate PR to try and fix it. #772
yarn install v1.22.11
[1/5] Validating package.json...
error [email protected]: The engine "yarn" is incompatible with this module. Expected version "1.22.10". Got "1.22.11"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
Error: Process completed with exit code 1.

Thanks! Please let me know if there was something done incorrectly from my end. Will fix it 🙂

jenweber

I think my PR worked! We'll see. I'll merge your PR in a moment if tests are green.

Replace jQuery animation for table of contents and remove jQuery package

6842c54

swarajpure commented Jul 31, 2021

View reviewed changes