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

Merged
merged 22 commits into from
Sep 30, 2024

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?

@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.

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

This is very unfortunate, I definitely want to look into removing some of the text debt of this repository. Mainly the code style, I already opened #135 but due to personal reasons I didn't have the time to properly finish it (and tbh I kinda forgot about it at some point). But I'd like to pick that up and also change the build process to something more modern than snowpack as I am starting to have issues with it recently (One example of that is in the Matrix channel).

If it is needed I could definitely see myself being a supporter for the next two years while I still study at the TUD. I don't think I can allocate enough time to pick up some of the bigger projects like Telemetry/backend. But I also experimented with safari support recently and I definitely think it could be very easily implemented, potentially leveraging a GitHub CI runner for builds.

I know this is all kind of off topic for this PR but if you'd like me to be a supported for this project I'd be happy to help out :)

@OliEfr
Copy link
Member

OliEfr commented Sep 23, 2024

Off-Topic

Hi, some of the topics listed in the ToDos are definitely a bit over the top. I need to clean them up soon, or probably re-name that page "Project Ideas" on the website. Any contribution that you do is well received @A-K-O-R-A ! I think having someone as a CoreTeam member of the project who is still a member of TUD would be very beneficial. I could go ahead and add you as a CoreTeam member @A-K-O-R-A .

@fugidev @C0ntroller Maybe one of you can comment on the snowpack problem posted in the matrix-channel? If you don't have access anymore, I copied the screenshot into an Issue.


On-Topic

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.

I like all of that.

Some feedback:

  • Default setting: You set the default of the setting to enabled, but dont set it here and here? It should be set there in any case.
  • Settings: From what you describe, it appears that the feature is stable and will in the worst case not destroy the Selma table and change grades unintentionally. However: as we are still modifying an official Selma grade table, I think that the students should be aware of that. (Besides, its good publicity for TUfast.) So I think its important to inject a small text above the table saying that we made the modifications, similar to HisQis here. You can also mention your name there, of course. The reason why to have the table layout switch right above the table would be the same: if something breaks the students can switch easily. Such a setting logic is available here. I think it would only be copy-paste, but could be added in a later PR.
  • Feature naming: I see what you mean by jExam-Layout. Most students at TUD however don't know what jExam is (all Non-CS students). Maybe it would be better to rename the feature to "Notenstatistiken für Selma" oder "Selma verbessern". I would also be fine with the way it currently is and would merge it, but I think I have a slight preference towards renaming it and wanted to put the discussion forward.

Given that many students use TUfast, its always somewhat important to keep the many users in mind.

@Noxdor in the original message of the PR the following question regarding the icons was mentioned. Can you comment on that?

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 :)

@Noxdor
Copy link
Member

Noxdor commented Sep 23, 2024

@OliEfr Yes I can comment on that :)

First of all, thank you so much @A-K-O-R-A for contributing! It's great to see this extension not being dead and that there are still new developers interested in contributing, I really appreciate it! The changes look amazing, so let's give that setting a fresh new icon 😉

We are using the Phosphor Icons Family for all non-TUD icons inside the not so new anymore settings page. Inside settings.json, where you correctly added a new entry for a new settings tile, the icon is configured via the icon property. The value of that property is Ph<IconNamePascal>. The Ph comes from Phosphor, the icon library. The <IconNamePascal> is the name of the icon you want to be displayed. We are using @dnlsndr/vue-phosphor-icons to get a Vue component for each icon available in the icon family. It might be that some newer additions to the icon family are not available since we didn't update that package in forever, but you should be able to choose from most phosphor icons.

To find the right icon, I suggest you follow that link and just browse or search for the icon you want for that setting. Then you can just click on the icon, click on the Vue tab and copy the text that is marked green in the following screenshot:
image

Paste it as the icon property's value of the setting tile you added and done!

If you have any other questions, feel free to just ask!

@Noxdor
Copy link
Member

Noxdor commented Sep 23, 2024

This is very unfortunate, I definitely want to look into removing some of the text debt of this repository. Mainly the code style, I already opened #135 but due to personal reasons I didn't have the time to properly finish it (and tbh I kinda forgot about it at some point). But I'd like to pick that up and also change the build process to something more modern than snowpack as I am starting to have issues with it recently (One example of that is in the Matrix channel).

If it is needed I could definitely see myself being a supporter for the next two years while I still study at the TUD. I don't think I can allocate enough time to pick up some of the bigger projects like Telemetry/backend. But I also experimented with safari support recently and I definitely think it could be very easily implemented, potentially leveraging a GitHub CI runner for builds.

I know this is all kind of off topic for this PR but if you'd like me to be a supported for this project I'd be happy to help out :)

I am currently still at TUD, but I won't be from ~ December on, which I think means that all original maintainers are not TUD students anymore. It would be amazing if you could take over some control and maybe even find some interested other students that would pick up some tasks. It would be a shame if this extension disappears, since I think it is pretty widely used and very useful, at least it was to everyone I spoke with about.

Regarding the build process, I 100% agree, it would be great to move to something like vite and maybe even switch out npm&node with bun, making some of the TypeScript development easier. I wanted to rework the popup a while ago, but like the others, I am not finding the time for it anymore :/ I think things like telemetry are very very secondary, Safari would be a great addition though.

@Noxdor
Copy link
Member

Noxdor commented Sep 23, 2024

One more thing: I also can't test it since I am not getting my grades in selma but HISQIS (Hence I implemented that table back in the day).

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

Default setting

The setting is now enabled by default and I added the corresponding lines to the background.ts file, the setting was also renamed to improveSelma internally and in the Settings page to "Selma vebessern".

I also added a small text and button to toggle the feature as seen in the pictures below

image
image

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

A-K-O-R-A commented Sep 30, 2024

In addition to the current feature set I would also merge a feature to display the users own grade as someone has opened a pull request for exactly this feature on my fork at A-K-O-R-A#2. Though I would prefer to do that in a separate pull request as there are some merge conflicts and small changes I want to do before integrating that feature.

If you have any other suggestions let me know, I feel like this is now in a state that is ready to be merged :)

@Noxdor
Copy link
Member

Noxdor commented Sep 30, 2024

In addition to the current feature set I would also merge a feature to display the users own grade as someone has opened a pull request for exactly this feature on my fork at A-K-O-R-A#2. Though I would prefer to do that in a separate pull request as there are some merge conflicts and small changes I want to do before integrating that feature.

If you have any other suggestions let me know, I feel like this is now in a state that is ready to be merged :)

Again, I can't test the changes live since I don't have this data inside selma, but you absolutely appear to know what you're doing and the screenshots looks great, so from my side, I agree this can be merged now 👍 I also think it's a good idea to handle the other topic in a separate PR. I find it interesting though that you got the pull request on your fork instead of us getting it here directly 😂

@OliEfr
Copy link
Member

OliEfr commented Sep 30, 2024

I see you addressed all the changes, thank you very much. It also seems good to me. I will merge this to main and find the time this week to publish the changes.

You forgot to set the flag on install here, which I fixed. LMK if that is problematic, but I dont see how.

@OliEfr OliEfr merged commit 2a55bdc into TUfast-TUD:main Sep 30, 2024
1 check passed
@A-K-O-R-A
Copy link
Contributor Author

Totally fine, I probably forgot it while renaming the property. Thank you for merging :)

@OliEfr
Copy link
Member

OliEfr commented Oct 1, 2024

I uploaded the new version to the webstores. Publishing should take couple of days.

@t0mbrn
Copy link

t0mbrn commented Oct 1, 2024

In addition to the current feature set I would also merge a feature to display the users own grade as someone has opened a pull request for exactly this feature on my fork at A-K-O-R-A#2. Though I would prefer to do that in a separate pull request as there are some merge conflicts and small changes I want to do before integrating that feature.
If you have any other suggestions let me know, I feel like this is now in a state that is ready to be merged :)

Again, I can't test the changes live since I don't have this data inside selma, but you absolutely appear to know what you're doing and the screenshots looks great, so from my side, I agree this can be merged now 👍 I also think it's a good idea to handle the other topic in a separate PR. I find it interesting though that you got the pull request on your fork instead of us getting it here directly 😂

I created the pr on the fork of the fork because I wanted, the feature wasnt merged back then. ill move the PR here

@OliEfr OliEfr mentioned this pull request Oct 1, 2024
9 tasks
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.

5 participants