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

Add an option for jExam like styling in Selma #149

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

A-K-O-R-A
Copy link
Contributor

@A-K-O-R-A A-K-O-R-A commented Sep 19, 2024

Description

I propose the following changes in my PR:

  • Add an optional alternative layout for the Exam views of Selma
  • Add grade distribution graphs
  • Add an indicator that shows the amounts of tries already used on a module

The following before and after images visualize the change:

image
image

image
image

image
image

Type of change

  • Bug fix (non-breaking change which fixes a bug)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that might cause existing functionality to not work as expected)

Further info

  • This change requires a documentation update
  • I updated the documentation accordingly, if required
  • I commented my code where its useful

Testing

We have 1500+ Users. Please test your changes thoroughly.

  • Tested my changes on Firefox
  • Tested my changes on Chromium-Based-Browsers
  • Successfully run npm run test locally

Additional Information

The changes need to be turned on in the settings page, as of now the setting has the same Icon as the Opal settings. A different icon would probably be better but I am unfamiliar with the way the icons work so a bit of help would be appreciated :)

I also accidentally reformatted the src/manifest.firefox.json file, I hope this is not an issue

@A-K-O-R-A A-K-O-R-A requested a review from a team as a code owner September 19, 2024 20:50
@OliEfr
Copy link
Member

OliEfr commented Sep 19, 2024

This looks awesome, thanks a lot!

A couple of questions on my side, as I am not at TUD anymore and out of the loop regarding Selma. I will have a closer look at the PR next week.

  • the grade distribution is for an exam among all participating students, I assume? Where do you query this data?
  • I am of the opinion that if a feature is a strict improvement, what your graphs are, it could be enabled by default.
  • with the hisqis table we had the enabled/disable option right above the table (not in the TUfast settings page). This was good as it's clear for the user how to enable / disable and prevents some implementation huzzle in the TUfast backend.

@A-K-O-R-A
Copy link
Contributor Author

Grade distribution

image

Selma provides a small button that opens a popup with a full table listing every grade with its count. The script extracts this link, downloads the html content and extracts the data from the table by querying the DOM. I'll leave comments to the relevant code sections below.

The grade distributions seem to be grouped by birth year, though I am not 100% sure of this as I have very limited data, as of now I can say that two students who started the same degree in the same year and wrote the same exam at the same time seem to see the same grade distribution. But for example if student A writes an exam in his second semester together with student B who writes the exam in his 4th semester the grade distributions shown will be different. Even though they wrote the exact same exam on the same day.

Enabling by default

For now I'd just enable the setting by default. Adding a button directly on the page is definitely a good idea so I'll try to implement this when I find the time for it :)

Besides all this I have another question, right now this PR merges into main as the develop branch is currently 7 commits behind. Should I still change it to merge into develop like it is advised in the contribution guide?

promises.push(
fetch(url).then(async (s) => {
const parser = new DOMParser()
const doc = parser.parseFromString(await s.text(), 'text/html')
Copy link
Contributor Author

@A-K-O-R-A A-K-O-R-A Sep 20, 2024

Choose a reason for hiding this comment

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

The page contains script elements diretly after the link elements that contain the code to open the Popup. By using a RegExp the arguments for this URL get extracted from the script to then construct the URL that would get opened in the PopUp when clicking on the icon
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracting the individual tries works pretty much the same, extracting the information is just a bit more complicated and unreliable. I am not 100% happy with how it is working right now as it still has trouble with some modules that format their information tables differently. For example language courses don't work as of now as shown in the picture below. Solving this issue definitely is not hard I just do not have the time right now :)

image

The individual tries also show the grade and date when hovering over them, the grade distribution charts also have tooltips showing what grade the bar represents. All graphics still act as clickable button that opens the original link just like before, though I am not sure if this immediately obvious and there might be a better solution to indicate to users that the charts are clickable

@C0ntroller
Copy link
Member

C0ntroller commented Sep 20, 2024

as I am not at TUD anymore and out of the loop regarding Selma.

This is also a problem for me. We need new supporters but no one wants to do it and also develop the necessary updates :/ Sadly, I can't check this PR.

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

Successfully merging this pull request may close these issues.

3 participants