-
-
Notifications
You must be signed in to change notification settings - Fork 23
Added functionality to show content of a merge request and navigate the merge request diffs #76
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
base: master
Are you sure you want to change the base?
Conversation
@@ -6,7 +6,7 @@ | |||
transform-origin: top left; | |||
border-radius: 0px 0px 4px 4px; | |||
cursor: pointer; | |||
z-index: 10; | |||
z-index: 1000; |
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.
needed by some very high z index on the diff tree of gitlab
window.location.href = (window.location.origin + "/" + URLDetails.dirFormatted + "/-/merge_requests/" + mergeRequestId + "/diffs#" + hash); | ||
} else { | ||
window.location.hash = hash; | ||
window.location.reload(); |
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.
overkill if you ask me but somehow the window did not refresh when changing the hash only
@@ -1 +1,142 @@ | |||
export { default } from "./TreeItem"; | |||
|
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.
copied the whole function from google, as I was not able to import crypto sha1 into node.js somehow. I bet you can, then you can remove this entire block
{isMergeRequestShown() ? ( | ||
<div className="spantree-filter-header"> | ||
<input type="checkbox" id="filterTests" name="tests" /> | ||
{/* <input type="checkbox" id="filterTests" name="tests" onClick={handleChange} defaultChecked={checkedTests} /> */} |
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.
I could not get it to work that the checkbox state is properly stored and the checkbox is checked... the idea of these checkboxes is to filter out unwanted changes from the flat tree of changes
map['renamed'] = document.getElementById('filterRenamed').checked; | ||
map['imports'] = document.getElementById('filteredImports').checked; | ||
map['newFile'] = document.getElementById('filteredNewFiles').checked; | ||
return map; |
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.
this 'backend' logic works as soon as the checkboxes are checked and the state of these is fixed properly
@@ -9,7 +9,7 @@ | |||
.spantree-tree-list { | |||
scroll-behavior: smooth; | |||
overflow-y: auto; | |||
height: calc(100vh - 40px); | |||
height: calc(100vh - 90px); |
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.
less space due to the 50px of the filter area.
Maybe there is a way to have -40 for repository view and -90 on merge request view, I wasnt able to do this without copy the entire css style and make it conditional on the pane...
let data = action.payload['changes']; | ||
if (action.filtersEnabled['test']) { | ||
data = data.filter((node) => { | ||
return !node.new_path.includes('src/test/'); |
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.
does not add any entries related to tests
} | ||
if (action.filtersEnabled['renamed']) { | ||
data = data.filter((node) => { | ||
return !node.renamed_file; |
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.
does not add files to the list if they were renamed
} | ||
if (action.filtersEnabled['removed']) { | ||
data = data.filter((node) => { | ||
return !node.deleted_file; |
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.
does not add files to the list if they were deleted
} | ||
if (action.filtersEnabled['newFile']) { | ||
data = data.filter((node) => { | ||
return !node.new_file; |
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.
does not add files to the list if they were added
} | ||
if (action.filtersEnabled['imports']) { | ||
data = data.filter((node) => { | ||
return !node.new_file; |
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.
functionaltiy not implemented yet: I wanted to filter all entries where the change was simple import changes. ie.:
(removed line) import a.b.c.d.EClass
(added line) import f.g.h.IClass
This information can be grepped by the rest call but processing it is most likely overkill.
That information is stored on node.diff as a string, it is a delta diff of the files and cumbersome to use.
I bet there are some libraries that could deal with that...
If you cannot make it work, scrap this one and remove the corresponding checkbox
@@ -1,6 +1,6 @@ | |||
import { SET_WIDTH } from "../../types/UI"; | |||
|
|||
const initialWidth = 250; | |||
const initialWidth = 300; |
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.
made a bit bigger to show all checkboxes
@@ -5,6 +5,7 @@ | |||
"requires": true, | |||
"packages": { | |||
"": { | |||
"name": "span-tree", |
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.
needed?
Hi
I have added a toggle view also for merge requests, where the developer can see the files received from rest API of gitlab, and when clicking on it, the corresponding diff view is opened. This comes in very handy for big merge requests, as gitlab has issues displaying them.
It is best if the user sets to show a single file in the diff view in his user preferences.
Issues that I could not resolve due to my limited knowhow of react and your project