Overhaul project summary page #859
Conversation
…ary-view-for-better-visualisation
…r-visualisation' of github.com:qbicsoftware/data-manager-app into feature/#812-improve-the-project-summary-view-for-better-visualisation
There was a problem hiding this comment.
Heya @sven1103! Thanks for the awesome overhaul! I have some notes outside of missing JDs and more general questions which should be addressed:
A.) Experiment creation on the project page is currently broken with the following error message:
2024-11-11 17:29:16,368 ERROR [http-nio-8080-exec-10] l.q.d.e.UiExceptionHandler: Missing level info for experiment.created.success. java.lang.RuntimeException: Missing level info for experiment.created.success. at life.qbic.datamanager.views.notifications.MessageSourceNotificationFactory.parseLevel(MessageSourceNotificationFactory.java:156)
B.) Optional Values cannot be removed during the edit process, this includes the following optional values:
1.) Person responsible
In this case the dialog does not allow the user to remove both values for the person responsible without showing an invalid state during validation.
2.) Funding Grant and Funding ID
This fails with the following Error message.
2024-11-11 17:42:08,253 ERROR [http-nio-8080-exec-4] l.q.d.e.UiExceptionHandler: No value present java.util.NoSuchElementException: No value present at java.base/java.util.Optional.orElseThrow(Optional.java:377) at life.qbic.datamanager.views.projects.project.info.ProjectSummaryComponent.lambda$buildFundingInformationSection$1832a10e$1(ProjectSummaryComponent.java:339) at com.vaadin.flow.component.ComponentEventBus.fireEventForListener(ComponentEventBus.java:239) at com.vaadin.flow.component.ComponentEventBus.fireEvent(ComponentEventBus.java:228) at com.vaadin.flow.component.Component.fireEvent(Component.java:411) at life.qbic.datamanager.views.projects.edit.EditFundingInformationDialog.onConfirmClicked(EditFundingInformationDialog.java:75)
C.) The load time of the new project summary is actually insanely long in comparison to the previous structure. I prepared a comparison here (with HO VPN)
A) Old project summary on RDM machine
Screen.Recording.2024-11-11.at.17.48.45.mov
**B) Project Summary overhaul on localhost **
Screen.Recording.2024-11-11.at.17.49.03.mov
Finally, Bonus Points are awarded if you also adress the bug i found:
The dialogs are not closed if the user navigates via the browser back and forward buttons. (However this is also happening before the overhaul)
Screen.Recording.2024-11-11.at.17.45.41.mov
| /** | ||
| * <b><enum short description - 1 Line!></b> | ||
| * | ||
| * <p><More detailed description - When to use, what it solves, etc.></p> | ||
| * | ||
| * @since <version tag> | ||
| */ |
| /** | ||
| * <b><class short description - 1 Line!></b> | ||
| * | ||
| * <p><More detailed description - When to use, what it solves, etc.></p> | ||
| * | ||
| * @since <version tag> | ||
| */ |
| /** | ||
| * <b><class short description - 1 Line!></b> | ||
| * | ||
| * <p><More detailed description - When to use, what it solves, etc.></p> | ||
| * | ||
| * @since <version tag> | ||
| */ |
| /** | ||
| * <b><class short description - 1 Line!></b> | ||
| * | ||
| * <p><More detailed description - When to use, what it solves, etc.></p> | ||
| * | ||
| * @since <version tag> | ||
| */ |
| /** | ||
| * <b><class short description - 1 Line!></b> | ||
| * | ||
| * <p><More detailed description - When to use, what it solves, etc.></p> | ||
| * | ||
| * @since <version tag> | ||
| */ |
| /** | ||
| * <b><class short description - 1 Line!></b> | ||
| * | ||
| * <p><More detailed description - When to use, what it solves, etc.></p> | ||
| * | ||
| * @since <version tag> | ||
| */ |
| /** | ||
| * <b><class short description - 1 Line!></b> | ||
| * | ||
| * <p><More detailed description - When to use, what it solves, etc.></p> | ||
| * | ||
| * @since <version tag> | ||
| */ |
| .flex-horizontal { | ||
| display: flex; | ||
| flex-direction: row; | ||
| } |
There was a problem hiding this comment.
I'm unclear on the purpose of the all.css class?
I guess that we're moving into having all css into a singular file as @KochTobi suggested or is it css classes that are used in general components? Maybe a small comment would help her?
| var projectId = context.projectId() | ||
| .orElseThrow(() -> new ApplicationException("No project id provided")); | ||
| var projectOverview = projectInformationService.findOverview(projectId) | ||
| .orElseThrow(() -> new ApplicationException("No project with given ID found")); | ||
| var fullProject = projectInformationService.find(projectId) | ||
| .orElseThrow(() -> new ApplicationException("No project found")); |
There was a problem hiding this comment.
Since we don't differentiate between individual reloads, you could also call the reloadInformation() method here which will rebuild all the information.
| var toast = notificationFactory.toast("project.updated.success", | ||
| new String[]{project.getProjectCode().value()}, getLocale()); | ||
| toast.setDuration(Duration.ofSeconds(5)); | ||
| toast.open(); |
There was a problem hiding this comment.
Since the toasts should behave the same independent on which section is edited this could be moved into its own method which is called by all buildSection methods?
There was a problem hiding this comment.
TBH, i think the toasts should behave equal for the same type in the complete app. I agree with you, that it makes no sense to configure them on a custom base every time we create them.
|
@Steffengreiner thank you for your Review! Regarding the loading times: i think you have tested two different projects, and the one you showed with the new design has tons of experiments, that need to be loaded to create the experiment design overview. So i think this comparison is not reflecting an issue based on the implementation change. I will look into the other found issues though. |
I dont see how this is related to my PR, since I worked on the project summary use case. If you can prove me wrong, go ahead, otherwise I kindly ask you to create a separate issue for it. |
|
Heya @sven1103, you're right! I investigated the issues and can confirm that the experiment creation was already broken before this PR. I created an issue Concerning the load times, you're also correct that this overhaul did not change it and it's due to the number of experiments within a project. I think this also has to be tackled seperately since the ACL mechanism seems to be the reason for this loading time. Finally i also created a seperate issue for the dialogs not closing if the user navigates via the browser buttons. |
…ary-view-for-better-visualisation
There was a problem hiding this comment.
Nicely Done! 👍
As a final note if you want to go for the cherry on top, you could also check the sonarcloud issues
for easy to fix outstanding issues such as unused imports/methods and variables.
|
sven1103
deleted the
feature/#812-improve-the-project-summary-view-for-better-visualisation
branch
No description provided.