Report Viewer Redesign #1145
Report Viewer Redesign #1145
Conversation
|
Currently, I get a blank page on https://kr0nox.github.io/JPlag/. Is that only for me the case? |
|
@tsaglam Should be up again |
There was a problem hiding this comment.
So, I finally found time to look through this giant PR, inspect the code and test a few things.
Besides that, Timur also played around quite a bit with the new Report Viewer.
I think the code is already in a quite good state. Besides the new design (which is already really good!), you simplified and enhanced a lot of things, thank you.
I also added some code comments, e.g., in the factories. Although I know this is not your code, we should still improve it in future versions :)
In addition to the comments in the code, I have the following suggestions (for later):
- The Light/Dark mode toggle button is hard to see, maybe it could be included as a toggle switch in the header, like on the vue.js website for example?
- The coloring of the comparison table with red lines is too much, I would suggest using a way lighter highlighting color, e.g., some shade of gray (pun intended)
- I wonder whether the back navigation from the comparison view should be possible via the GUI and not only via the browser (same e.g. for the more button)
- Overall there should be more in-code documentation of public methods and classes
- Underlining headlines is a relic from ancient typewriter times and should be avoided :)
- At least for scrolling with the mouse wheel was not possible in the comparison view top compartment
- The warning about missing submissions (because of the limit) should be visible both in the comparison list and the cluster list (e.g., a cluster with low confidence shows 10 participants but listing them all shows only one entry)
- Sometimes, matches require two clicks to become visible in the comparison view
In favor of quickly merging this PR, I would suggest not directly implementing the suggestions (other than the easy-to-implement ones in the code) but moving them to issue #1000 and then implementing them afterward.
Again thank you for your work. I'm looking forward to merging this PR!
|
|
||
| /** | ||
| * Factory class for creating Comparison objects | ||
| */ | ||
| export class ComparisonFactory { | ||
| public static getComparison(id1: string, id2: string) { | ||
| console.log('Generating comparison {%s} - {%s}...', id1, id2) |
There was a problem hiding this comment.
We should discuss whether we want console logs in a normal productive run without errors
| const comparisons = [] as Array<ComparisonListElement> | ||
|
|
||
| const metrics = json.metrics as Array<unknown> as Array<Record<string, unknown>> | ||
| // Average |
There was a problem hiding this comment.
Could be split up in functions
There was a problem hiding this comment.
I would put his into a new PR since we are planning on changing something in the json to make it easier to implement new metrics. Then this code will have to change anyways
There was a problem hiding this comment.
Yes, please regard all of my comments as things that could also be done in an additional PR to speed up the process :)
| /** | ||
| * Gets the overview file based on the used mode (zip, local, single). | ||
| */ | ||
| public static getOverview(): Overview { |
There was a problem hiding this comment.
Also a quite complex method especially the construction of the Overview object. Something to rework in the future
|
All still open comments are either points that need to be disgussed in more detail or will be handle in a different PR.
We dont really have a header like that. we could add one, but at the moment it would only contain that switch.
Is done
Was removed
It is possible on the trackpad by scrolling to the side. Do you have an example of any website that allows horizontal scrolling with the mouse wheel?
How excactly do we want that displayed?
This investigating where that issue comes from @tsaglam Dragging should be possible again |
As far as I can see, this is already an issue on the develop branch. @sebinside can you confirm that this is the case for you too? |
|
@Kr0nox Yes, the issue did already exist earlier. As far as I see, all other points are minor or can be discussed in the next meeting. So, when you feel ready (and the conflicts are resolved), I'll be happy to merge the PR. |
|
One small thing, either for this PR, or a future one: The top list in the overview should have indexed pairs again because that helps to keep track of pairs when the search function is used. |
# Conflicts: # report-viewer/package-lock.json # report-viewer/package.json
Do you mean just numerating the lines or giving every comparison a unique id? |
Just numbering the lines. So when I filter for a submission name, I see exactly at which positions the comparisons involving that submission are in the unfiltered list. |
|
@tsaglam is implemented |
|
[JPlag Report Viewer] Kudos, SonarCloud Quality Gate passed! |
|
[JPlag Plagiarism Detector] Kudos, SonarCloud Quality Gate passed!
|
|
@sebinside Conflicts are resolved. From my side this PR can be merged |
This PR gives the UI of the Report Viewer a complete overhaul.
It is designed to be more user friendly, constistent and easier to extend.
It only adds minimal new features to stay in line with the old design and not display less information.
Changelist
Packages
Tailwind
Font Awesome
Removed
General
Views
FileUpload
Overview
Comparison
Cluster
Information
Model
MetricobjectDistributionobject ArrayComparisonArrayStore
uiStateandstateTesting
A deployed version can be found here