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

Rework the feedback table #5102

Merged
merged 33 commits into from
Nov 12, 2023
Merged

Rework the feedback table #5102

merged 33 commits into from
Nov 12, 2023

Conversation

bmesuere
Copy link
Member

@bmesuere bmesuere commented Nov 3, 2023

This pull request partly reworks the feedback table according to #5101. The main goal was to make the debug button more prominent. In the old design, this was not possible since the feedback level that was associated with the debugger (group) did not have any UI.

Changes

Cards with header

Every group now has its own card with a header. The header contains an incremental number of the group/test, the status in words, a debug button if relevant, and a button to collapse the card. Correct cards are collapsed by default, except if everything is correct. The toggle button that was used to hide correct "tests" was repurposed to collapse/expand correct cards.

image
image

Tab summary

In between the tabs and the feedback table, we showed 0, 1 or 2 toggle buttons in the old design. The empty space at the left is now used to add a summary of that tab. Every card is represented by a colored circle, indicating its status. These circles are clickable and link to the corresponding card. The card then scrolls into view, the border size is increased to 2px and expands if needed.

The height of this summary is fixed (to the height of the toggle buttons) and will show a vertical scroll bar if there would be too many tests.

image
image

Other changes

  • The tabs + summary are now "sticky", which means they'll stay visible when scrolling down
  • The lower test levels were left unchanged for now. I just tweaked the spacing a bit and moved a status icon from the right to the left.

Implementation

Most of the changes are done in the feedback table renderer. Some of the html that is generated there could in theory move to javascript to reduce the time that is needed to generate a server response. This could probably be done in a later PR as we'll probably merge the pythia renderer at a later time.

Collapsing the cards is almost entirely done in css by adding/removing the collapsed class.


If you test this, please try exercises with complex feedback tables. Since our format is very flexible, there are probably edge cases which I have missed.

@pdawyndt
Copy link
Contributor

pdawyndt commented Nov 3, 2023

You may find some place to integrate these features at the card level:

  • toggles (now students have to scroll back to the top of the page to perform that action)
  • copy testcase description to clipboard (= stdin, statements/expressions)

@pdawyndt
Copy link
Contributor

pdawyndt commented Nov 5, 2023

Lena and me prefer option 3. We find extra border for 2 too hard. Green/red text on blue background doesn’t blend so well.

@bmesuere
Copy link
Member Author

bmesuere commented Nov 5, 2023

For showing the group status, I made a few alternatives:

  1. Show an icon + status in bold and in the status color. Bgcolor of the card is light blue for all, target card gets dark blue border:
    image

  2. Same as 1, but also set the border to the status color. Target card gets a 2px border.
    image

  3. Change the header color to the status
    image


I don't like 3 (in part because I never liked the light green for areas). 1 is maybe to subtle, 2 seems like a good middle ground.
On a busy page, this might become too busy. An alternative might be to only add the border for wrong groups.

I'll implement 2 for now.

@bmesuere
Copy link
Member Author

bmesuere commented Nov 5, 2023

Lena and me prefer option 3. We find extra border for 2 too hard. Green/red text on blue background doesn’t blend so well.

My screenshot was a bad example. On "real" pages, the prominent background interferes too much with the diff background (like here). They are both "large areas with the same red/green" color, but have a different function (failing "similar things should look the same, similar looking things should function the same). The group status is not that important that we want such explicit focus (failing the squint test). Correct groups will be collapsed by default which will make the different enough.

@pdawyndt
Copy link
Contributor

pdawyndt commented Nov 5, 2023

Not sure about the “collapse correct by default” yet, but may have to see it in practice. My fear is that will not be very supportive for students if the main focus is on what’s wrong, by hiding what is already OK.

@bmesuere bmesuere added feature New feature or request deploy mestra Request a deployment on mestra labels Nov 5, 2023
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Nov 5, 2023
@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Nov 5, 2023
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Nov 5, 2023
@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Nov 5, 2023
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Nov 5, 2023
@jorg-vr
Copy link
Contributor

jorg-vr commented Nov 6, 2023

I approve of the new layout, this is a nice step forward!

I do have some notes on the colors:

  • I prefer the light grey outline border. It makes the cards a lot less "heavy" on the page. Otherwise they al seem focused
  • The blue color does not go well with the red and green. This is a problem in al three designs, as there is either red/green on blue or the reverse
  • I greatly prefer the third design, that most clearly shows the test state without an ugly hard border. I would even allow the buttons in this design to be green/red. Maybe design 1 could also work if the header background wasn't blue.

@bmesuere
Copy link
Member Author

bmesuere commented Nov 9, 2023

Thanks for the feedback. Here are the updates I've implemented:

Scrollbars: This is indeed very ugly on windows (I didn't spot this while testing). I now set the scrollbar-width to none for this specific element, but this only works in Firefox for now. For webkit browsers, I used a pseudo selector to hide the scrollbars in this element. In case these two approaches fail, I've also restricted overflow to the y-axis, which should remove the horizontal scrollbar and alleviate most of the concern. @niknetniko I decided on using a fixed height (and thus scrolling the little area) to be able to support scrolling the cards into view without javascript. A fixed top scroll margin is set in css for this (otherwise the card scrolls to the top of the page and is hidden by the navbar and other sticky elements).

Card borders: I reverted the colored borders and once again use the divider color (light gray). For targeted cards, the border is set to the primary color (dark blue). The width stays at 1px (as opposed to the 2px of the previous design).

Multi-line alignment: This was a bug. I didn't test with descriptions that spanned multiple lines

image

Regarding the changes I've not implemented:

Update the icon font: This would require making the current font even bigger. It seems that iconify might be an alternative. I'll try to add this in a separate PR. We could then gradually migrate all icons.

Card header color: I tried all dodona css variable colors as header and almost none "worked". Most were too prominent or didn't fit. Others (like the proposed darker gray) interfered with the other gray areas in the card content. I decided to stay with blue for now. The removal of the colored borders will probably also help make everything easier on the eyes.

Tweak margins and padding: Increasing the content density by reducing white space doesn't seem necessary to me. Part of the goal of this PR was to make the hierarchy and content of the table clearer and reducing the density was an important part in this. In the previous design, you also had to scroll to see the entire table, so the difference is in the amount of scrolling. This is countered by the addition of the top summary which means that in many cases users won't have to scroll at all. However, I'm open to making adjustments to the spacing if we identify any regressions or UI glitches, such as with the multi-line descriptions. Because of the nested nature of the table and optional text and code that can show up almost everywhere, it's easy to miss a few cases.


I'll do a few more tests on naos to see if I missed any cases.

@niknetniko
Copy link
Member

I did a test with a bunch of messages on multiple levels, and they all seem to work:

image

Tweak margins and padding

It bothers me a bit that the padding on the left and right are not equal (probable with the status icon being there on the left), but that could just be me.

@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Nov 10, 2023
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Nov 10, 2023
@jorg-vr
Copy link
Contributor

jorg-vr commented Nov 10, 2023

There is a bug with the left padding in this example: https://naos.dodona.be/nl/submissions/15701527/#tab-1-group-9
image

I like the new borders, with the primary color for targeted cards.

I find it rather counter intuitive that it is impossible to close the targeted card.
User flow: jump to a card, study it, close when finished to focus on the one below => hmm this does not work? Feels like a bug.

It is also impossible to remove focus from a card, except navigating to another one. I know this it caused by the target implementation but it doens't feel natural. (Not a priority to fix, would have bothered me less if I could close the card)

The navigation help does not give any indication which one is already targeted. Definitely with al icons being the same (either wrong or correct) it can be easy to lose your position. I've noticed the tooltips after writing this => they do help with this problem, although it is still not optimal.

Card header color: I tried all dodona css variable colors as header and almost none "worked". Most were too prominent or didn't fit. Others (like the proposed darker gray) interfered with the other gray areas in the card content. I decided to stay with blue for now. The removal of the colored borders will probably also help make everything easier on the eyes.

Did you also try background-color: rgba(var(--d-primary-rgb), 8%); as I suggested?
I must say that in light mode the problem of the page being to heavy is already resolved for me in the current iteration. But in dark mode it still bothers me. But this can just be an unresolvable question of taste, so I am willing to accept as is.

@bmesuere
Copy link
Member Author

I have tried setting the "currently selected card" by using :focus or :active, but both don't seem to work reliably. I think you would also expect something scroll-spy like for this. If you click an icon, the page scrolls to that card. If you then continue scrolling, it might be confusing if the clicked icon was still highlighted.

If we were to add more JavaScript in a next iteration, we could opt to:

  • add a scrollspy (or highlight the clicked icon)
  • manually expand the focused card instead of using css (to allow it to be collapsed again)
  • dynamically compute the header height and remove the fixed height from the summary icons

@chvp chvp added the deploy naos Request a deployment on naos label Nov 12, 2023
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Nov 12, 2023
@bmesuere bmesuere merged commit 08e138b into main Nov 12, 2023
14 of 15 checks passed
@bmesuere bmesuere deleted the feature/feedback-table-rework branch November 12, 2023 14:58
@pdawyndt
Copy link
Contributor

pdawyndt commented Nov 12, 2023

I was in the midst of evaluating a test when the new feedback table was rolled out, and now I feel seriously hampered by the fact that by default all the correct contexts are collapsed (and I also have no way to expand them all). I'm loosing quite some overview, especially because the bash judge (then one whose results I'm currently evaluating) puts every individual test in its own context. I'm now feeling terribly slowed down while evaluating submissions.

Example: https://dodona.be/nl/feedbacks/752542661/

@bmesuere
Copy link
Member Author

You can expand them all by clicking the top eye button:
image

We'll investigate what the best way forward is for evaluations (storing the user preference vs expanding).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants