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

Fixes notification width on Students page #5278

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

Bones5
Copy link

@Bones5 Bones5 commented Jun 16, 2022

Fixes #5094

Changes proposed in this Pull Request

  • Adds wp-header-end to position notification correct
  • Switches to grid for custom navigation display

@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Merging #5278 (79ab61e) into trunk (8213374) will increase coverage by 0.05%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #5278      +/-   ##
============================================
+ Coverage     40.70%   40.75%   +0.05%     
- Complexity     8604     8616      +12     
============================================
  Files           284      284              
  Lines         29012    29042      +30     
============================================
+ Hits          11809    11837      +28     
- Misses        17203    17205       +2     
Impacted Files Coverage Δ
includes/admin/class-sensei-learner-management.php 2.23% <0.00%> (ø)
...-api/class-sensei-rest-api-messages-controller.php 86.66% <0.00%> (+0.45%) ⬆️
...api/class-sensei-rest-api-questions-controller.php 84.15% <0.00%> (+2.34%) ⬆️

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 12bc69a...79ab61e. Read the comment docs.

Copy link
Member

@m1r0 m1r0 left a comment

Choose a reason for hiding this comment

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

Hey, I'm a bit late, but thanks for looking into this!

I've left a few notes but let me know if you have any questions/suggestions.

flex-flow: column wrap;
align-items: flex-start;
gap: 20px;
display: grid;
Copy link
Member

Choose a reason for hiding this comment

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

My suggestion here would be to keep the previous code, because I think it would be better to move the notifications outside of the navigation. That way we won't have to account for them being there. See the next comments for more info.

Copy link
Member

Choose a reason for hiding this comment

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

Forgot to mention that the grid view changes how the navigation looks on the other screen (some of the spacing is missing), but we won't have to worry about that if we decide to move the notifications outside.

@@ -123,7 +122,7 @@ public function add_custom_navigation() {
if ( ! $screen ) {
return;
}

Copy link
Member

Choose a reason for hiding this comment

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

I think the empty space here makes the linter angry. 😄 Could you remove it?

Comment on lines +138 to +148
<div id="sensei-custom-navigation" class="sensei-custom-navigation">
<div class="sensei-custom-navigation__heading-with-info">
<div class="sensei-custom-navigation__title">
<h1><?php esc_html_e( 'Students', 'sensei-lms' ); ?></h1>
</div>
<div class="sensei-custom-navigation__separator"></div>
<a class="sensei-custom-navigation__info" target="_blank" href="https://senseilms.com/documentation/student-management?utm_source=plugin_sensei&utm_medium=docs&utm_campaign=student-management">
<?php echo esc_html__( 'Guide To Student Management', 'sensei-lms' ); ?>
</a>
</div>
<div class="sensei-custom-navigation__separator"></div>
<a class="sensei-custom-navigation__info" target="_blank" href="https://senseilms.com/documentation/student-management?utm_source=plugin_sensei&utm_medium=docs&utm_campaign=student-management">
<?php echo esc_html__( 'Guide To Student Management', 'sensei-lms' ); ?>
</a>
<hr class="wp-header-end">
Copy link
Member

@m1r0 m1r0 Jul 12, 2022

Choose a reason for hiding this comment

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

What do you think of moving the .wp-header-end element away from the navigation (reverting this change) and directly to the students inner page? My idea is to move it right after where the navigation would end up after it's moved by the JS code. I think in this case the element could be moved in these two files:

Let me know if you have other ideas!

@m1r0 m1r0 added the [Status] Needs Author Reply Requires response from the author label Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs Author Reply Requires response from the author [Type] Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Student Management: Notices should be full width
2 participants