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

Make center divider adjustable #403 #418

Merged
merged 31 commits into from Jan 29, 2019
Merged

Conversation

emer7
Copy link
Contributor

@emer7 emer7 commented Dec 12, 2018

Fix #403

RepoSense has a code panel and summary panel, each occupies
half of the screen. However, the code panel width cannot be resized.

Enabling resizing of the code panel will allow user to adjust it to
their comfort.

Let's have an adjustable code panel by dragging the width of the panel.

reposense 2
This is implemented by constantly changing the inline style following cursor movement.
I avoid direct DOM manipulation.

Copy link
Contributor

@ongspxm ongspxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't work on firefox, chrome too. i just clicked on the black bar and dragged, is it the right way to use it?

it doesn't really move, it just selects the text.

2ov1ql

@emer7
Copy link
Contributor Author

emer7 commented Dec 13, 2018

Thank you for the comment. Right now it cannot be dragged very fast. The cursor need to stay in the black bar for the dragging to work.

Future plan: finding a way to allow more responsive dragging or allow dragging despite cursor not in the black bar.

@damithc
Copy link
Collaborator

damithc commented Dec 13, 2018

Thank you for the comment. Right now it cannot be dragged very fast. The cursor need to stay in the black bar for the dragging to work.

hmm... we need something that works for all users when used as per normal user expectations. Can look for an alternative that doesn't have that restriction?

@ongspxm
Copy link
Contributor

ongspxm commented Dec 13, 2018

one way to do it is to do it relative to the global div (more specifically #app-wrapper), attach the mousemove event on #app-wrapper, moving the separator if a certain flag is triggered (this triggering of the flag will be attached to the mousedown and mouseup event of the bar, same to what you have now).

@emer7
Copy link
Contributor Author

emer7 commented Dec 13, 2018

yeah I thought of someting similar just now, using another div wrapper but turns out there is #app-wrapper. I will check on it. Thank you for the suggestion

@emer7
Copy link
Contributor Author

emer7 commented Dec 13, 2018

I will work on it on Sunday night, GMT+2 (currently in Israel on NOC). Unavailable until then. I apologize for the absence.

@emer7
Copy link
Contributor Author

emer7 commented Dec 16, 2018

Fixed the issue above. Now the dragging is more intuitive.

Copy link
Member

@eugenepeh eugenepeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the issue above. Now the dragging is more intuitive.

Looks good! Just 1 more thing though, can give it a minimum width or make it auto close when the code view has less than a certain width?

image

For e.g., I have accidentally slide too far right and now I couldn't adjust it anymore.

@@ -13,7 +13,7 @@ function toggleNext(ele) {
}

parent.className = classes.join(' ');
};
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for lines that are irrelevant to your PR, please avoid modifying them as it may cause unnecessary conflict and also deviates from the purpose of your PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes from ESLint

frontend/src/static/js/v_summary.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ongspxm ongspxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using the flex prop is a smart trick 👍

it will be best if are could work on the lag, maybe the double rendering on both .tab-resize and app-wrapper might be the cause, do give a more detailed look.

@@ -13,7 +13,7 @@ html
script(src="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/9.12.0/highlight.min.js")
body
div#app
#app-wrapper(v-if="userUpdated")
#app-wrapper(v-if="userUpdated", v-on:mousemove="mouseMove", v-on:mouseup="deregisterMouseMove", v-bind:style="{ 'user-select': appWrapperUserSelect }")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, newline pls, just break on commas

@@ -26,7 +26,8 @@ html
strong RepoSense
span   on {{ creationDate }}

#tabs-wrapper(v-if="isTabActive")
#tabs-wrapper(v-if="isTabActive", v-bind:style="{ flex: `0 0 ${flexWidth * 100}%` }")
.tab-resize(v-on:mousedown="registerMouseMove", v-on:mousemove="mouseMove", v-on:mouseup="deregisterMouseMove")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same and why did you have to do it in two place tho? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I thought that without it, we can only register the mouse event outside of the resize bar

window.innerWidth - (DRAG_BAR_WIDTH / 2)
) / window.innerWidth;

this.flexWidth = calculatedWidth;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just this.flexWidth = ..., dun need set const then set again.

Copy link
Contributor

@ongspxm ongspxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to deal with the lag, you can do some event buffering mechanism, so that the heavy lifting at the front can be avoided.

@emer7
Copy link
Contributor Author

emer7 commented Jan 27, 2019

Proposed commit message

RepoSense has code panel and summary panel, each occupies
half of the screen. However, the code panel width cannot be resized.

Enabling resizing the code panel will allow user to adjust the panel
size to their comfort.

Let's have an adjustable code panel by dragging the width of the panel.

@eugenepeh
Copy link
Member

sorry @emer7 , i just realized something:

When I adjust the divider position, hiding the code panel and show it again, it resets to the middle position. Only when I interacts with the dashboard again, it goes back to the adjusted position.

Can you look into it to see if we can let it maintain the adjusted position through out?

@emer7
Copy link
Contributor Author

emer7 commented Jan 27, 2019

sorry @emer7 , i just realized something:

When I adjust the divider position, hiding the code panel and show it again, it resets to the middle position. Only when I interacts with the dashboard again, it goes back to the adjusted position.

Can you look into it to see if we can let it maintain the adjusted position through out?

Done, width now persists

mouseMove = throttledEvent(30, innerMouseMove);
};

const deregisterMouseMove = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lint your code, migth not be able to do this, do window.deregisterMouseMove instead, same above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is this pesky #514 that is going to go in, it will fail your code, so lint on local first

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed const xxx = ... to window.xxx = ... but I don't lint everything

@yamidark yamidark requested review from ongspxm and damithc and removed request for ongspxm January 28, 2019 16:58
Copy link
Collaborator

@damithc damithc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work @emer7 👍

@damithc
Copy link
Collaborator

damithc commented Jan 29, 2019

Thanks to reviewers too, for shepherding this PR all this way 💯

@yamidark yamidark merged commit 73e47bf into reposense:master Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants