Fix notices positioning

[fjorgemota]

Fixes #5581. Alternative to #5585, but very specific to Learning Mode (so if #5585 is approved/merged, this isn't needed).

I did this because the implementation on #5585 uses JS, which may not be....desirable. So this PR just avoids using JS, but instead just fixes the issue on Learning Mode only, which also may not be desirable.

Changes proposed in this Pull Request

• Fixes notices positioning, but only on Learning Mode;

Testing instructions

Follow the testing instructions on #5581, and make sure the notices are now positioned correctly.

Screenshot / Video

[codecov[bot]]

Codecov Report

Merging #5586 (40cb853) into trunk (0ff1e2c) will decrease coverage by 0.00%. The diff coverage is 47.09%.

@@             Coverage Diff              @@
##              trunk    #5586      +/-   ##
============================================
- Coverage     44.88%   44.87%   -0.01%     
- Complexity     8882     8883       +1     
============================================
  Files           430      430              
  Lines         31787    31795       +8     
  Branches        266      266              
============================================
+ Hits          14268    14269       +1     
- Misses        17322    17329       +7     
  Partials        197      197              

Impacted Files	Coverage Δ
...s/admin/editor-wizard/steps/course-upgrade-step.js	72.72% <ø> (ø)
.../admin/editor-wizard/steps/lesson-patterns-step.js	33.33% <ø> (ø)
assets/blocks/quiz/quiz-block/quiz-timer-promo.js	100.00% <ø> (ø)
assets/shared/blocks/settings.js	18.18% <0.00%> (ø)
includes/admin/class-sensei-learner-management.php	2.21% <0.00%> (ø)
includes/admin/class-sensei-learners-main.php	4.96% <0.00%> (ø)
...des/block-patterns/class-sensei-block-patterns.php	27.69% <0.00%> (ø)
...e-list/class-sensei-course-list-block-patterns.php	0.00% <0.00%> (ø)
...cludes/blocks/class-sensei-block-quiz-progress.php	0.00% <0.00%> (ø)
includes/blocks/class-sensei-blocks.php	21.53% <0.00%> (ø)
... and 117 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 868fa9e...40cb853. Read the comment docs.

[yscik]

Added a task to style it, thanks for the ping! https://github.com/Automattic/sensei/issues/5679

[fjorgemota]

Hey @renatho, thanks for your review and sorry for the late answer.

Just replying your question:

I checked that notices seem to be working correctly on other pages outside of Learning Mode, but maybe I missed something? If there are no other issues, I prefer this solution instead of https://github.com/Automattic/sensei/pull/5585.

The notices do appear, but with this PR they appear in the wrong position, at the top of the page.

On #5585, the notice "Lesson Reset Successfully." appears like this on Twenty Twenty One:

(I zoomed out the result here, by the way)

Meanwhile, with this PR, the same notice appears like this on Twenty Twenty One:

Is it clear what I mean here? :)

[dadish]

@fjorgemota @renatho There is a fix (#5746) for "notice positioning only on Learning Mode" issue. So the issue is fixed for pages in Learning Mode. For lessons and quizzes.

Though the issue with the notice showing up on top of the page still stands and it is happening on all themes, both classic and FSE themes.

[fjorgemota]

@dadish Hey there, thanks for the ping.

Now I don't know exactly what to do.. :shrug:

This PR is intended to fix the issue with the notices showing on the top of the page, and it does that by not running maybe_print_notices on the wp_body_open action and instead running it directly on the block sensei-lms/course-theme-notices, which is used with Learning Mode.

I also worked on an alternative approach, #5585, where I use JS to put the notices in the "right" place. That generally works well with both FSE and classic themes, from what I tested, but it still conflicts quite a bit with the PR you mentioned.

Do you have any suggestions about what to do? Just close this PR given that your PR is quite similar, and maybe consider an approach like the one in #5585, or....I don't know?

Thanks.

[dadish]

@fjorgemota

I would say the issue #5586 is fixed. So this PR probably could be closed.

The issue with the notice showing up too early I think still stands. And it can be dealt separately.

[yscik]

add_action( 'wp_body_open', array( $sensei->notices, 'maybe_print_notices' ) );

I don't think outputting the notices in the wp_body_open hook (nor in init which was the issue before) makes sense, since that's not where we want them. the_content seems like a good candidate instead.

There is already a method to do that for notices from blocks: Sensei_Notices::prepend_notices_to_content, being added from Sensei_Notices::setup_block_notices. Since the original intent in notices to the init step was to have the notices everywhere, I'd suggest to use this one for everything:

add_filter( 'the_content', [ $sensei->notices, 'prepend_notices_to_content' ] );

And remove the others:

add_action( 'wp_body_open', array( $sensei->notices, 'maybe_print_notices' ) ); Sensei_Notices::setup_block_notices and add_action( 'template_redirect', [ $this, 'setup_block_notices' ] );

[fjorgemota]

@dadish Ok, closing then.

@yscik I thought about adding to the_content, but I wasn't sure if that filter runs on every page that needs notices, so that's why I didn't add to that hook. Either way, I'm closing this PR as you can take this on your project anyway, as it's already doing some changes in Learning Mode anyway. Does that sound reasonable? :)

[gikaragia]

As we are close to our code freeze and the notice being on top of the page exists some time now, I think that we should handle this as part of Janitorial.

Maybe this deserves a new issue but I think that it is clear what we need to do with this one from this comment.

[aaronfc]

Closing this PR again since there is already a ticket (linked) for working on the final solution for this. Adding a reference to latest comment from @gkaragia there.