Skip to content
This repository was archived by the owner on Apr 1, 2020. It is now read-only.

Bugfix/quickopen sizing #1030

Merged
merged 7 commits into from
Nov 29, 2017
Merged

Conversation

akinsho
Copy link
Member

@akinsho akinsho commented Nov 28, 2017

@bryphe, I managed to get the flickering to stop at smaller widths, granted my solution is not the best.

I added maxWidth to the styles to prevent the fullWidth prop alternating, however I ran into an issue where the left positioning very rarely was being set to a negative value so I check its value and return it as a positive int.

Very open to any changes you might have, I did test it out locally a fair bit and it seems to work without any strange edge cases nor do I think the general functionality has changed but not sure if there's something somewhere I might have missed.

Also Added overflow scroll on the element so the user can scroll to end of that element as they can for one beneath it although I didn't do much with the scrollbar styling except make it smaller to keep it out of the way. This bit is likely not necessary and/or desirable so happy to remove it.

possibly fixes #1020

@@ -148,10 +148,15 @@ export class CursorPositionerView extends React.PureComponent<ICursorPositionerV
visibility: this.props.hideArrow ? "hidden" : "visible",
}

const leftSidePositioning = (x: number) => Math.sign(x) !== -1
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this could be simplified with Math.abs

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the tip @bryphe, tweaked it to use Math.abs, much simpler!

right: this.state.isFullWidth ? "8px" : null,
maxWidth: "95%",
Copy link
Member

Choose a reason for hiding this comment

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

Cool, seems like this fixes the issue!

@bryphe
Copy link
Member

bryphe commented Nov 28, 2017

Change looks great @Akin909 , thank you for taking a look at this! That sizing logic is not trivial.

I left some minor feedback on the PR (we might able to use Math.abs to simplify the sign-change for the x-position), but overall looks good to me.

@bryphe
Copy link
Member

bryphe commented Nov 29, 2017

Awesome, change looks good!

The AutoCompletion-CSS test is a new test that isn't stable - not related to your change. I'm working on a fix in PR #1032 . I'll bring this in now - thanks for the contribution, @Akin909 !

@bryphe bryphe merged commit e8aa37e into onivim:master Nov 29, 2017
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.

Tooltip continuously resizing when oni less than fullscreen
2 participants