Skip to content
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 ability to expand/collapse modules #3628

Merged
merged 5 commits into from Sep 23, 2020
Merged

Conversation

gikaragia
Copy link
Contributor

Changes proposed in this Pull Request

  • Adds a chevron in the module title which expandes/collapses the module details.

Testing instructions

  • Open up the editor end add a module.
  • Test that collapsing expanding works.
  • Open a course with modules in the frontend.
  • Repeat the tests.

@gikaragia gikaragia requested a review from a team September 18, 2020 13:27
@renatho
Copy link
Contributor

renatho commented Sep 18, 2020

@jom suggested at some point that we could use the summary/details, so we could use the same approach to the editor and frontend. Just to confirm, Did we try to explore this option?

@gikaragia
Copy link
Contributor Author

@jom suggested at some point that we could use the summary/details, so we could use the same approach to the editor and frontend. Just to confirm, Did we try to explore this option?

Not sure if we can have CSS based animations with the details element. It seems that elements of varying height are hard to animate with CSS. I'll have a look on Monday.

@renatho
Copy link
Contributor

renatho commented Sep 18, 2020

@jom suggested at some point that we could use the summary/details, so we could use the same approach to the editor and frontend. Just to confirm, Did we try to explore this option?

Not sure if we can have CSS based animations with the details element. It seems that elements of varying height are hard to animate with CSS. I'll have a look on Monday.

Yeah, if we want to animate the height, it can be hard without JS. I saw that you used max-height: 1000px;, but I think it can be a little risky. If we want animate, we could find some alternative animation, like using translate + scale + opacity, or something like that.

@gikaragia
Copy link
Contributor Author

Yeah, if we want to animate the height, it can be hard without JS. I saw that you used max-height: 1000px;, but I think it can be a little risky. If we want animate, we could find some alternative animation, like using translate + scale + opacity, or something like that.

Hey @renatho I think that we should avoid JS based animations as CSS ones are easily overriden by the user. For this I followed this guide. As explained there the issue with transforms is that no reflow is triggered so the rest of the elements won't move accordingly. We could possibly trigger a reflow but this could cause performance issues.

I increased the default max height to 2000px, it seems that it doesn't affect the animation too much. This would allow for 26 lessons to be added to a single module without max-height being an issue. I imagine that if this becomes an issue for the user, they could always tweak the animation again or remove it entirely.

@jom jom changed the base branch from add/progress-preview-button to master September 21, 2020 14:37
@yscik
Copy link
Contributor

yscik commented Sep 21, 2020

Generally it should be fine to have Javascript drive the animation, performance-wise. In this case, it's probably overkill — maybe we can consider it later if we have heavier animations/interactions on the frontend and decide to include some library for it.

We could add the initial max-height (or the class containing it) from JS though, so if it's disabled, the CSS isn't there either — approaching it as progressive enhancement. But that can be in a PR when we add the option to disable it :)

@gikaragia
Copy link
Contributor Author

@gikaragia gikaragia requested a review from a team September 23, 2020 10:17
.children( '.wp-block-sensei-lms-collapsible' );

moduleDetails.toggleClass( 'collapsed' );
jQuery( this ).toggleClass( 'dashicons-arrow-up-alt2' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add dashicons as a dependency for the frontend stylesheet. It isn't loaded by default for most themes. For example, logged out (admin bar and query monitor both require this), this does not display on Storefront.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use an svg icon compiled in at some point, but that's worth its own PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! 5e8e3b8

We should probably use an svg icon compiled in at some point, but that's worth its own PR

Considered using CSS content attribute but found it a bit hacky.

@@ -184,7 +185,7 @@ public function render_course_outline_block( $attributes ) {
$block_class .= ' ' . $attributes['className'];
}

return '
return '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whitespace will get stripped in my editor due to our .editorconfig. Was it added for a reason?

Copy link
Contributor Author

@gikaragia gikaragia Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope 😄 Not sure why PHPStorm doesn't respect it, I have the plugin installed. Probably has to do with my project structure 🤷 0e2d7f8

Copy link
Contributor

@yscik yscik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for this, all looks good! ✈️

@yscik
Copy link
Contributor

yscik commented Sep 23, 2020

Found a problem with Wordpress 5.3, the frontend JS is not loaded. Maybe the registration is too late in Sensei_Frontend? Tried moving it to an enqueue in enqueue_block_assets and that seems to work, but that won't pick up the Disable JS setting. We can tackle this in another PR, since we also need to adjust the block CSS loading to respect the Disable Sensei CSS option.

@donnapep donnapep added this to the 3.6.0 milestone Sep 28, 2020
@donnapep donnapep changed the title Add expand/collapse button in modules Add ability to expand/collapse modules Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants