move feedback message to top of feedback card

[Alex-Jordan]

This moves the feedback message to the top of the card, as discussed in the #939 thread.

I'm not sure about it yet. Additionally styling adjustments might be in order.

[drgrice1]

This looks good to me. What additional styling adjustments do you thing are needed? I don't see anything that looks wrong to me.

[Alex-Jordan]

I can't put my finger on it. The transition from the message to the h4 below it sometimes seems off to me, depending on the size of the message. And maybe it matters that the headers below (h4) feel heavier than the h3 at the top of the card.

Maybe it's how the yellow message background color has 1:1 contrast ratio with the gray color of the h4 below it. That might explain what I see with the transition from the message to the header below it.

Anyway, I'm not really sure. I'm fine with just leaving it, while still wondering if others see what I see and have ideas.

[drgrice1]

None of it looks that off to me, but would it help if there were a black border on the bottom of the feedback message? Perhaps also on the top of the feedback message? Or perhaps just a border all around the feedback message?

[Alex-Jordan]

Yeah, a border on the bottom of the feedback really helps.

Is it better implemented as a CSS border or an hr? I'm leaning toward an hr because I can simply exclude it when there is no message, or when there is a message but there is nothing further below the message.

[drgrice1]

Whatever you think works best.

Another option I thought of is an inset box shadow.

[drgrice1]

Also, it isn't hard to implement a border on the bottom with css that is only there if the message is not the last child.

[Alex-Jordan]

I added a bottom border with just CSS.

[drgrice1]

Looks good.

I noticed another issue though (not caused by this pull request). I wanted to test the case that there is nothing below the message. So I used the following problem:

DOCUMENT();
loadMacros('PGstandard.pl', 'PGML.pl', 'parserMultiAnswer.pl',);

$ma = MultiAnswer(0)->with(
	singleResult      => 1,
	allowBlankAnswers => 1,
	checker           => sub {
		my ($cor, $stu, $self, $ansHash) = @_;
		$self->setMessage(1, 'message that is always shown');
		return 1;
	}
);

BEGIN_PGML
[__________]{$ma}
END_PGML
ENDDOCUMENT();

I noticed that the feedback message card body does not get the border radius of the popover, and so the background color doesn't look the best.

[Alex-Jordan]

Hmm, if you click to "Preview", then there is no Correct/Incorrect overall title. And then you get like:

I noticed that the feedback message card body does not get the border radius of the popover, and so the background color doesn't look the best.

Is this about the gray border around everything in the pic above?

[drgrice1]

No, it is just the background color, and it is not just in this pull request. It is how it already is. If you add

					&:last-child {
						border-bottom-left-radius: 0.5rem;
						border-bottom-right-radius: 0.5rem;
					}

To the feedback message it is a little better. That is the border radius of the popover, but it still doesn't quite match up right.

[drgrice1]

To see this check answers with that problem without entering an answer.

[Alex-Jordan]

I had to look real hard to see that. I think my red/green color blindness blurs the yellow and gray together a bit and I have to get pretty close to the screen to see the radius issue. That blurring of yellow and gray is probably what was bothering me before the black border too.

[Alex-Jordan]

What should happen when there is a message, but no overall header? Like in the screenshot with Preview Answers?

[drgrice1]

Hmm, I am not sure. That is a problem introduced with this pull request. Maybe we just need to make sure there is always a header. So for a preview, perhaps make the title "Preview Only".

[Alex-Jordan]

I fixed the radius, I think. It seemed that I needed to use calc(0.375rem - 1px) instead of 0.5rem.

I added a header for when the popover is just preview.

And I noticed the cursor over the main header was only turning to a pointer when the feedback was "correct". I changed it to always be a pointer on the feedback card header, regardless of feedback type.

[Alex-Jordan]

I fixed the radius, I think. It seemed that I needed to use calc(0.375rem - 1px) instead of 0.5rem.

OK, I think I figured out it should be var(--bs-card-inner-border-radius) and I changed it to that, with a force push.

[drgrice1]

There is a problem now for essay questions. The problem is that the PGessaymacros.pl macro sets the resultClass to the empty string. Then line 1198 of PG.pl makes the data-bs-custom-class attribute equal to "ww-feedback-popover ". The space at the end of that is a problem, because bootstraps javascript splits that string on spaces, and calls classList.addClass on the array that it gets from the split. In this case that will be ['ww-feedback-popover', '']. The empty string in that array causes an error because it is not a valid token. The error message is shown in the console, and the popover doesn't work.

To fix this, I suggest that instead of adding the "preview" class, revert to allowing the resultClass to be the empty string, but make the css default for the popover header to have what you added for the preview style. So move lines 227 and 228 in htdocs/js/Problem/problem.scss up to line 223 (in the base .popover-header definition), and put the resultClass handling in PG.pl back to the way it was.

It might be a good idea to add a comment to the POD regarding the resultTitle stating that this should not be set to the empty string.

It is important to remember with these settings used in PG.pl for this that custom answer types (like the essay answers) can override any of the options set in the %options hash initially defined on line 1023 of PG.pl.

[Alex-Jordan]

OK, I think I addressed that now.

This is unrelated to this PR, but ideally an essay answer title would not always be "Ungraded" and the message would not always be "This answer will be graded at a later time." Ideally it would recognize that it's been assigned a score since the last time an answer was submitted, and it would give responses based on that.

[pstaabp]

This looks nice. I like the message at the top. Also, tested an essay question and I think the last commit fixed the error @drgrice1 pointed out.

[drdrew42]

This is looking really nice -- I much prefer having the feedback message at the top 👍

[drgrice1]

This is now set to the empty string by default again.