-
Notifications
You must be signed in to change notification settings - Fork 113
New frontend remaining issues #615
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
Conversation
|
@mdeme01 Thanks for the quick work, we will review it! For the remaining issues, please create a PR / issue, so the review process is simplified. (The changeset is smaller and it is more clear what was changed and for what reason.) |
…into new-frontend-issues
This problem occurs because you did not specify an 'src' label that is the path to the source directory in the codebase of the project. Because all of the projects had this label on the demo website, I thought this label was mandatory for every project, apparently, it is not. I used the 'src' label for the Metrics, because if you click the root directory, or any directory above the source folder, it still generates the Metrics for the source folder, as seen on the demo website, and I took examples from there during development. The problem is that generating metrics when clicking a sub-folder (represented by a rectangle in the treemap) is dependant on the path of the folder, because it works by appending/removing the folder name to the path. And if you generate the metrics for anything outside the source folder, the result will still be the metrics for the source folder, and the path will be out of sync. For example, generating it for the root folder So this is why, when clicking a directory outside the source folder, I appended the source folder path to the current path (stored in the 'src' label), because I did not know how else to solve this. (And I could not figure out how this works in the old frontend.) |
Unfortunately the existence of the
To generate the metrics for a file or a directory:
This is how it is done in the current UI. |
|
#613 (Make labels dynamic on New Frontend): the labels are recognized and displayed correctly on the new UI, but clicking them raises an error in the console log:
The path is valid and correct on my dev environment, the label also works on the old UI correctly. UPDATE: I got the same error when checking the metrics, as you have mentioned it depends on it. |
mcserep
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the mentioned issues with the labels, the changes look good to me and functionally sound.
Yes, I know that. I explained the problem above. I'm sorry if it wasn't clear, I will try to explain it again. If you click a directory for metrics, you need its ID, this much is correct and clear, and it works that way correctly. However, after the metrics has been generated, you need to be able to generate metrics for other directories, displayed in the treemap. The treemap is generated from the metrics result, which only contains the names of the directories, not their path. You should be able to generate metrics for a directory by clicking on it inside the treemap. This operation requires a new path, for a new metrics, but you can only work with the name of the clicked directory, you don't know its path. But you have the path of the directory you initially generated the metrics for, so you can just append the name of the directory to this path. For example: you generated metrics for the The problem comes in when you generate metrics for anything outside the source folder, like the For some reason |
I checked it, for me it works correctly. I used the cJSON project and specified 3 labels. I used the following command: (In my case, the project is located in All 3 labels are displayed and clicking them navigates to the correct directory. Are you sure the path you specified is correct for the that project? |
Yes, it works on the old UI as well. I will try to debug this later. |
@mdeme01 I have debugged this and the source of the error is that in the DB the path is: Note the trailing slash. Therefore |
The The old UI simply does a smart move and if the metrics for |
Okay, I misunderstood it, and I didn't realize it when I first implemented it. Thanks for clarifying. Now I think I was able to fix it, at least it is working for me, so it does not require the |
Thanks for the update and the fix, it works now for me as well similarly like in the old UI.
This is the only remaining issue I see before merging this PR. |
I didn't explicitly find out how this is handled in the old UI. However, I added a check for the trailing slash, and if the path contains one, it passes the path without it to the request, and this seems to be working. |
The old UI processes the path by elements, splitting it by the directory separators. See in var path = shortcut.split('/');
path[0] = '/';
path.forEach(function (directory) {
if (!directory.length) // <-- this ignores the last element if empty
return;
// ...
});Since it is not required in the new UI, a simple check as you have implemented should be fine. |

This PR aims to fix the remaning issues related to the new frontend. @mcserep @intjftw
Issues fixed: