Selma styling updates #153

t0mbrn · 2024-10-01T16:57:51Z

Pull Request

Please fill out the following template when you create a PR. This will help to process your PR faster!

Description

I proprose the following changes in my PR:

Ability to show your own grade in the grade summary
Ability to toggle this behavior
Loading animation for grade bar chart
Default bar chart for unpublished exams/grade distributions

Here you can also add screenshots, if applicable.
Loading Animation:
https://github.com/user-attachments/assets/594efb7e-92b7-4692-9742-60f493cdaa84

New View turned on:

Settings Page

New View Turned off

References

I created the PR because I felt like its a nice addition

Referenced Issue:

Referenced ToDo from project-board:

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

Add anything here, that might be important.

t0mbrn · 2024-10-01T17:02:31Z

Moved this here

A-K-O-R-A · 2024-10-01T19:07:25Z

I really like the loading animation but after looking through the code it seems like you don't stop the animation if there is an error while parsing the grade table. I did not handle these errors and decided to just do nothing in that case as it was easier. But I think having users forever stuck in the loading animation without being able to click the original link is not a good option. You can add a .catch after the promises to handle upon errors, I think simply putting back the original button is good enough but maybe there are better suggestions.

The NULL_TABLE doesn't actually need to be this big, the function that creates the graph automatically reduces the input array of grades to a predefined subset of grades using the pickGradeSubset function so you can remove all the entries with a count of 0. I probably could have documented that better :)

The part where you parse the grade is a bit problematic because not everyone receives grades exactly formatted liek taht, for example some get a "be" which is then counted as a "1,0" in the grade statistic. I think simply doing gradeNum = parseFloat(gradeElm.replace(",", ".")) is more robust and NaN can then be treated differently :)

Besides that you can probably see that there are some merge conflicts, these are because of some changes I implemented before my original pull request got merged so unfortunately those have to be resolved, I'm sorry this all became a bit of a mess

t0mbrn · 2024-10-01T19:29:44Z

I really like the loading animation but after looking through the code it seems like you don't stop the animation if there is an error while parsing the grade table. I did not handle these errors and decided to just do nothing in that case as it was easier. But I think having users forever stuck in the loading animation without being able to click the original link is not a good option. You can add a .catch after the promises to handle upon errors, I think simply putting back the original button is good enough but maybe there are better suggestions.

I'll do the catch routine - maybe I'll add another placeholder with the link embedded for it I'll see

The NULL_TABLE doesn't actually need to be this big, the function that creates the graph automatically reduces the input array of grades to a predefined subset of grades using the pickGradeSubset function so you can remove all the entries with a count of 0. I probably could have documented that better :)

I saw that procedure but I was honestly too lazy to properly read it - will do

The part where you parse the grade is a bit problematic because not everyone receives grades exactly formatted liek taht, for example some get a "be" which is then counted as a "1,0" in the grade statistic. I think simply doing gradeNum = parseFloat(gradeElm.replace(",", ".")) is more robust and NaN can then be treated differently :)

Simple replace doesn't work as the grade is in predefined steps - at least in the course view where an average is calculated I need that parsing. But I'll add more safeguards to it.

Besides that you can probably see that there are some merge conflicts, these are because of some changes I implemented before my original pull request got merged so unfortunately those have to be resolved, I'm sorry this all became a bit of a mess

I'll reapply my changes to the main branch - maybe I'll do a fresh PR, or I'll merge it properly I'll see:)

OliEfr · 2024-10-01T20:31:09Z

I like the loading animation. Overall, I think the design changes to the Selma table are a good addition to the project.

Thanks @A-K-O-R-A for checking the technical details!

One opinion:
Why is there a toggle for showing the own grade in the first place? Is there any case where I do not want to see my own grade? The grade was shown before anyway, right? If there is a case where I do not want anyone to see my grades on-screen, I should just not log into Selma at that moment. I have a preference for removing the setting toggle entirely, also because of see below.

In case you decide to keep the settings flag:

Can you default enable it at the two locations mentioned here (first bullet point under "On-Topic")? I think its important to prevent issues on install and update. Secondary, its nice to have an overview of all setting variables.
Can you rename selmajExamThemeWG to improveSelmaWithGrade (reference)? A variable needing explanation is not a good one. There is no need for abbreviating.
I think technically you should only be able to enable the WithGrade if improveSelma is already activated. Having such sequential settings is always a bit of a pain in implementation. I didn't check all your code changes, but I am not sure whether you considered this. You can consider this issue either in the UI or backend logic. If you want to implement this, please refer to similar setting implementations in TUfast. It is confusing for the user if the second one can be activated without the first one.

t0mbrn · 2024-10-01T20:49:55Z

I like the loading animation. Overall, I think the design changes to the Selma table are a good addition to the project.

Thanks @A-K-O-R-A for checking the technical details!

One opinion:

Why is there a toggle for showing the own grade in the first place? Is there any case where I do not want to see my own grade? The grade was shown before anyway, right? If there is a case where I do not want anyone to see my grades on-screen, I should just not log into Selma at that moment. I have a preference for removing the setting toggle entirely, also because of see below.

It was requested on the Informatik Discord server - i personally dont see a reason for it either but apparently there is an audience for it.
Iirc it was about people becoming insecure because of the comparison to others or something

OliEfr · 2024-10-01T21:09:11Z

The entire idea of grading is to compare yourself to others.

OliEfr · 2024-10-01T21:16:32Z

Either way, having or not-having the toggle should not decrease user experience. I am ready to support both sides, and would also be happy to merge the feature without the toggle. It is something that can be added later on anyways. Also, if you do not want to see the (anyway existing) comparison visually, you do not need to enable the improved-selma-feature. Besides: the own grade is shown next to the graph anyways in case I interpret the screenshots correctly, no? (I cant check Selma myself unfortunately.) The more I think about it, the less I see the hard argument for this specific toggle.

t0mbrn · 2024-10-01T21:20:29Z

Either way, having or not-having the toggle should not decrease user experience. I am ready to support both sides, and would also be happy to merge the feature without the toggle. It is something that can be added later on anyways. Also, if you do not want to see the (anyway existing) comparison visually, you do not need to enable the improved-selma-feature. Besides: the own grade is shown next to the graph anyways in case I interpret the screenshots correctly, no? (I cant check Selma myself unfortunately.) The more I think about it, the less I see the hard argument for this specific toggle.

Yes. It is always shown next to it- I just removed it from my screenshots

OliEfr · 2024-10-01T21:31:52Z

Well, I contributed my opinion, and I am happy to argue with people who want to argue about this.

As @A-K-O-R-A and possibly your name directly appear in the credits of the table you should also take care that you feel comfortable with whats being published.

Dont get me wrong: I still think that the toggle is a strict improvement to TUfast, but only if my above points are addressed. I am sorry that this creates implementation overhead.

A-K-O-R-A · 2024-10-02T06:17:53Z

I think there are many students who do not want to compare themselves to others and pretty much only care about wether they have passed or not and that was the target audience I first thought about when asking for a specific toggle for this feature. But to be honest I did not stop to consider that most likely those people also do not want to see the whole distribution, so I would be fine with removing the toggle if it creates too much overhead.

For me it is also a visual thing as I don't really care about where my own grade lies in the distribution and rather look at the distribution to get an estimate of how hard the exam was and how different the grading patterns are (perfect Gaussian bell curves seem surprisingly rare)

Anyway I would let the decision up to you @t0mbrn if you want to keep the toggle, and add the additional logic that was requested, or remove it entirely. I am sorry for the additional work I caused :/

t0mbrn · 2024-10-03T12:47:18Z

@A-K-O-R-A which code style settings do you use? (is there something I can import to IntelliJ or vscode?) (is this the eslint file)?

t0mbrn · 2024-10-03T15:39:40Z

Can anyone help me out? 😭
I tried to develop the feature further, but when trying to install the (unchanged) manifest.json in firefox as temporary add-on, I always get:
"Während der temporären Installation des Add-ons trat ein Fehler auf.
Fehlerdetails

Extension is invalid

Reading manifest: Error processing background: Value must either: contain the required "page" property, contain the required "scripts" property, or not contain an unexpected "type" property"
i didnt get this before so im unsure how to proceed :/

OliEfr · 2024-10-03T16:12:33Z

Check Contributing.md for current style/linting guidelines. Can be improved in the future though. Also see #134.

Regarding error: Did you npm run useFF? Maybe promt ChatGPT-o1.

A-K-O-R-A · 2024-10-03T16:37:45Z

I use prettier in my editor and before every commit I run npm run test which will run eslint --fix and apply some formatting

t0mbrn · 2024-10-03T19:06:53Z

Check Contributing.md for current style/linting guidelines. Can be improved in the future though. Also see #134.

Regarding error: Did you npm run useFF? Maybe promt ChatGPT-o1.

uff danke
i have accumulated more then 3 instances of tu fast in my dev folder... apparently I didnt use it in my current working directory

thx

# Conflicts: # src/contentScripts/other/selma/layout.ts # src/freshContent/settings/Settings.vue # src/freshContent/settings/settings.json # src/styles/contentScripts/selma/exam_results.scss

t0mbrn · 2024-10-10T14:00:38Z

@A-K-O-R-A regarding

The part where you parse the grade is a bit problematic because not everyone receives grades exactly formatted liek taht, for example some get a "be" which is then counted as a "1,0" in the grade statistic. I think simply doing gradeNum = parseFloat(gradeElm.replace(",", ".")) is more robust and NaN can then be treated differently :)

currently only grades with a "," are parsed anyways - I could add an extra workflow for "be", if there is a need for it? I dont see the problem you mentioned - can you maybe elaborate more on it?

t0mbrn · 2024-10-10T14:05:46Z

Also, Could I add my GitHub tag behind the akora tag or would it get to cluttered?
Like Tom

t0mbrn · 2024-10-10T14:25:18Z

Unrelated:
there seems to be a css bug?

OliEfr · 2024-10-10T14:54:16Z

Yes, add your tag.

Also: can you quickly say which design / implementation choice you went for?

t0mbrn · 2024-10-10T14:55:44Z

Yes, add your tag.

Thanks

Also: can you quickly say which design / implementation choice you went for?

i removed the toggle

A-K-O-R-A · 2024-10-13T11:43:11Z

currently only grades with a "," are parsed anyways - I could add an extra workflow for "be", if there is a need for it? I dont see the problem you mentioned - can you maybe elaborate more on it?

For EBW courses simply present your own grade as "be" but mark them as a "1,0" in the grade distribution table.

t0mbrn · 2024-10-13T11:46:06Z

Unfortunately I don't have had such courses yet so I can't test it properly - which options are there besides grades with a ","
Only "be"?

A-K-O-R-A · 2024-10-13T11:51:15Z

I have only encountered "be" yet, but I can imagine that there might be more variations. Though I don't think it is really a problem if we don't handle them properly, as long as the rest works.

You can also see the graph moving left one column, I don't know if this is only an issue on my pc but it was consistently there on all pages. Mabye someone else can try it as well?

OliEfr · 2024-10-13T11:56:36Z

We cant try, unfortunately. (Not TUD anymore.)

I remember I had similar peculiarities and failure cases with the Hisqis table. Thats why I think its good that we have the disable switch right above the table. Implement it robust for the cases you can cover (maybe ask 1-2 fellow students to try out), and choose conservative routines for error handling.

t0mbrn · 2024-10-15T12:08:33Z

You can also see the graph moving left one column, I don't know if this is only an issue on my pc but it was consistently there on all pages. Mabye someone else can try it as well?

I could replicate this issue and will try to fix it

t0mbrn · 2024-10-15T12:31:31Z

weird - the disable table button doesnt work for me - or not as expected - it doesnt remove the improve selma stuff but it fixes this bug, when disabled?? @A-K-O-R-A

A-K-O-R-A · 2024-10-19T10:12:33Z

Mhm, unfortunately I do not have the time to look into this issue right now. It is definitely possible that I overlooked something when implementing the the disable button, maybe you can start removing things line by line from the function that creates this disable button until it works?

If you do not find the root cause then I can look into it on the weekend next week, sorry for the long delay :/

A-K-O-R-A and others added 18 commits September 18, 2024 13:03

prüfunge overview

e54a024

prüfungsergebnisse

013e210

diagram

fe6a264

fix diagram layout

3c3243e

reorganise code

5918cac

Add features to the courseresults page as well

1a54b90

Use document_idle instead of document_start

3fa00ce

Add href to grade overview

c39f024

Make everythin asynchronous to prevent lag

f69cb8f

Reorganise 'my exams' table

04b2647

Remove useless timespans from 'my exams' table

96f06cd

Add column title 'Notenverteilung'

6a97772

Map grade placeholders to emojis and add tooltips

5c1d7bb

Add try counter to course results

e0b9f65

Add setting to toggle the Theme

f64333d

Make tests pass

2ebff4b

show own grades

6bc40d7

added loading animation for bar chart

ff29281

t0mbrn marked this pull request as ready for review October 1, 2024 17:01

t0mbrn requested a review from a team as a code owner October 1, 2024 17:01

t0mbrn requested a review from OliEfr October 1, 2024 17:01

t0mbrn changed the title ~~Selma styling~~ Selma styling updates Oct 1, 2024

Tom added 2 commits October 10, 2024 15:43

removed toggle, added .catch for promises

848b441

Merge remote-tracking branch 't0mbrn/main' into selma_styling

42bfc98

# Conflicts: # src/contentScripts/other/selma/layout.ts # src/freshContent/settings/Settings.vue # src/freshContent/settings/settings.json # src/styles/contentScripts/selma/exam_results.scss

github tag, table header bug fixes

7b2000f

be grade handling, comments

fec7e49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selma styling updates #153

Selma styling updates #153

t0mbrn commented Oct 1, 2024 •

edited

Loading

t0mbrn commented Oct 1, 2024

A-K-O-R-A commented Oct 1, 2024

t0mbrn commented Oct 1, 2024

OliEfr commented Oct 1, 2024 •

edited

Loading

t0mbrn commented Oct 1, 2024

OliEfr commented Oct 1, 2024

OliEfr commented Oct 1, 2024 •

edited

Loading

t0mbrn commented Oct 1, 2024

OliEfr commented Oct 1, 2024 •

edited

Loading

A-K-O-R-A commented Oct 2, 2024

t0mbrn commented Oct 3, 2024

t0mbrn commented Oct 3, 2024

OliEfr commented Oct 3, 2024 •

edited

Loading

A-K-O-R-A commented Oct 3, 2024

t0mbrn commented Oct 3, 2024

t0mbrn commented Oct 10, 2024

t0mbrn commented Oct 10, 2024 •

edited

Loading

t0mbrn commented Oct 10, 2024

OliEfr commented Oct 10, 2024

t0mbrn commented Oct 10, 2024 •

edited

Loading

A-K-O-R-A commented Oct 13, 2024

t0mbrn commented Oct 13, 2024

A-K-O-R-A commented Oct 13, 2024 •

edited

Loading

OliEfr commented Oct 13, 2024

t0mbrn commented Oct 15, 2024

t0mbrn commented Oct 15, 2024

A-K-O-R-A commented Oct 19, 2024

Selma styling updates #153

Are you sure you want to change the base?

Selma styling updates #153

Conversation

t0mbrn commented Oct 1, 2024 • edited Loading

Pull Request

Description

References

Type of change

Further info

Testing

Additional Information

t0mbrn commented Oct 1, 2024

A-K-O-R-A commented Oct 1, 2024

t0mbrn commented Oct 1, 2024

OliEfr commented Oct 1, 2024 • edited Loading

t0mbrn commented Oct 1, 2024

OliEfr commented Oct 1, 2024

OliEfr commented Oct 1, 2024 • edited Loading

t0mbrn commented Oct 1, 2024

OliEfr commented Oct 1, 2024 • edited Loading

A-K-O-R-A commented Oct 2, 2024

t0mbrn commented Oct 3, 2024

t0mbrn commented Oct 3, 2024

OliEfr commented Oct 3, 2024 • edited Loading

A-K-O-R-A commented Oct 3, 2024

t0mbrn commented Oct 3, 2024

t0mbrn commented Oct 10, 2024

t0mbrn commented Oct 10, 2024 • edited Loading

t0mbrn commented Oct 10, 2024

OliEfr commented Oct 10, 2024

t0mbrn commented Oct 10, 2024 • edited Loading

A-K-O-R-A commented Oct 13, 2024

t0mbrn commented Oct 13, 2024

A-K-O-R-A commented Oct 13, 2024 • edited Loading

OliEfr commented Oct 13, 2024

t0mbrn commented Oct 15, 2024

t0mbrn commented Oct 15, 2024

A-K-O-R-A commented Oct 19, 2024

t0mbrn commented Oct 1, 2024 •

edited

Loading

OliEfr commented Oct 1, 2024 •

edited

Loading

OliEfr commented Oct 1, 2024 •

edited

Loading

OliEfr commented Oct 1, 2024 •

edited

Loading

OliEfr commented Oct 3, 2024 •

edited

Loading

t0mbrn commented Oct 10, 2024 •

edited

Loading

t0mbrn commented Oct 10, 2024 •

edited

Loading

A-K-O-R-A commented Oct 13, 2024 •

edited

Loading