Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Improved contextual toolbar positioning #120

Merged
merged 3 commits into from
Oct 17, 2012
Merged

Improved contextual toolbar positioning #120

merged 3 commits into from
Oct 17, 2012

Conversation

ncri
Copy link
Contributor

@ncri ncri commented Oct 15, 2012

Fixes the contextual toolbar to cover selections that are about to be edited.

In case there is a selection, move toolbar on top of it and align with
start of selection. Else move it on top of current position, center it and move
it slightly to the right.

Hope you like it!

In the future it would probably be best to also test for the case when the toolbar
will popup outside the visible area, like for text on the top of the client window,
and reposition it to a visible position.

A general question: I was building hallo.js with these changes before committing.
Shall I always do this or rather not check in new builds?

bergie added a commit that referenced this pull request Oct 17, 2012
Improved contextual toolbar positioning
@bergie bergie merged commit 2baa54c into bergie:master Oct 17, 2012
@bergie
Copy link
Owner

bergie commented Oct 17, 2012

@ncri thanks, it is indeed less "on the way" like this. But still, I'd rather have it below the selection, not above

@ncri
Copy link
Contributor Author

ncri commented Oct 17, 2012

Would you be okay with adding an option for changing it to above (default below)? I'd like to avoid having to use my own fork. :)

@ncri
Copy link
Contributor Author

ncri commented Oct 17, 2012

And maybe others like above as well...

@bergie
Copy link
Owner

bergie commented Oct 17, 2012

@ncri sure, why not. PRs welcome :-)

@ncri
Copy link
Contributor Author

ncri commented Oct 17, 2012

sure, ill do it ;-)

@ncri
Copy link
Contributor Author

ncri commented Oct 17, 2012

I could just add to this one if you reopen it, maybe good to keep the reasoning

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants