Improve check message

[ernilambar]

Tasks:

• Show check message above the result table (Fixes Message should show at top of screen. #200)

[ernilambar]

Before:

After:

cc: @swissspidy @felixarntz @mukeshpanchal27

[mukeshpanchal27]

Thank you for the PR. I have a concern regarding the approach taken in this PR. It might be beneficial for us to explore alternative options or discuss the approach within the issue before proceeding further.

[mukeshpanchal27]

This currently loops through similar data, requiring users to wait for all checks to complete before receiving results. For instance, in a large plugin with numerous Errors/Notices, users only see the results after all checks finish, displaying errors or successes. In my view, this negatively impacts user experience as users suddenly encounter a barrage of errors after a brief wait.

To observe this issue, you can compare the user experience of the WooCommerce plugin before and after the PR changes.

[ernilambar]

Yah, I noticed that in the bigger plugins. I have reverted that loop and kept as it is. To fix this, I have prepended the final output message instead of appending. If this approach is also acceptable then we can re-explore another approaches. :-)

[felixarntz]

While we don't use any custom CSS yet on this screen, that doesn't mean we shouldn't. I don't think a <br> tag is correct here, since it carries semantic meaning and is typically an inline element.

I think we should rather put an inline style attribute on one of the two divs, either with margin-bottom on the top one or with margin-top on the bottom one.

Alternatively, we could print a <style> tag in the head of the page, but that may be overkill for just this one style rule.

[ernilambar]

I have removed br tag and create separate PR to add inline styling. #368 Since this PR needs more discussion, we could merge that PR separately.

[felixarntz]

Not sure a separate PR is needed here. Using a <br> isn't quite right here semantically, and since there's now the more severe issue #341 (review) outlined by @mukeshpanchal27, which requires CSS to solve anyway, I think we should address this here.

[mukeshpanchal27]

Thank you for updating the approach.

The primary drawback is that the entire output shifts downward when an error message is appended.

During the checks, if someone starts reviewing the notices, once all checks are completed, the position of that initial error moves to the bottom due to new errors appearing at the top. Consequently, since there are no error messages at the bottom, if someone is inspecting the WC (WooCommerce) plugin and reaches the bottom, they won't find any notice. To address this, we should consider adding error messages at the bottom as well, allowing users to view them without having to scroll back to the top. What do you think?

[mukeshpanchal27]

Recoding for small plugin checks:

Screen.Recording.2024-01-02.at.11.57.46.AM.mov

[ernilambar]

To address this, we should consider adding error messages at the bottom as well, allowing users to view them without having to scroll back to the top. What do you think?

But also this wont fix the issue of shifting of output.

[felixarntz]

@ernilambar @mukeshpanchal27

To address this, we should consider adding error messages at the bottom as well, allowing users to view them without having to scroll back to the top. What do you think?

But also this wont fix the issue of shifting of output.

Both of this can be addressed. We can keep the notice only at the top, but we need to "reserve" the appropriate space for the notice. We can print a <style> tag on the page to make that happen (and also address #341 (comment)). Something like:

<style>
.plugin-check__results .notice:first-child {
	margin-top: # Whatever margin looks good
}

.plugin-check__results h4:first-child {
	margin-top: # Margin that is the same as the above margin plus the actual height of the notice
}
</style>

By doing that, the contents shouldn't shift any longer from the notice being added.

[ernilambar]

I have updated the PR with CSS. Currently I have added CSS to existing admin_footer() method because when I added new method to hook to admin_head, GitHub Action generate error like Too many public methods.

[felixarntz]

@ernilambar LGTM, just two small things below which IMO would be good to improve.

[mukeshpanchal27]

LGTM!