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

popup arrowStyle css property "left" is been overridden #36

Closed
yashhy opened this issue Sep 4, 2018 · 7 comments
Closed

popup arrowStyle css property "left" is been overridden #36

yashhy opened this issue Sep 4, 2018 · 7 comments

Comments

@yashhy
Copy link
Contributor

yashhy commented Sep 4, 2018

While trying to custom arrowStyle = { left: 20px } has no effect.

CodeSandbox link where the left: 50px is ignored
https://codesandbox.io/s/pmp848njwx

I see its getting overridden by this line of code here.

The Issue :

I set the offsetX props to move the popup content right (because that's my requirment) and now the arrow also moves right. So to centre align the arrow I use arrowStyle prop set the left property and that's been overridden.

Without OffsetX:

without_offset

Without OffsetX:

with_offset

Suggestions:

  1. Can we have a check if, the left property is explicitly set using arrowStyle, take the property from there without calculating arrowLeft
  2. Calculate the arrowLeft with respect to offsetX props. Like
    arrowLeft = ((triggerBounding.width / 2) - offsetX) + "px";. This would be bit buggy if the offsetX is set to very large values.
  3. Expose a extra prop offsetArrowX to specify the arrow left explicitly.

First approach is bit more cleaner and explicit.

Version

1.2.0

@yjose
Copy link
Owner

yjose commented Sep 4, 2018

HI, @yashhy good remark,
Can you open a pull request using the first approach !!

@yashhy
Copy link
Contributor Author

yashhy commented Sep 5, 2018

Sure will do 👍

@yashhy
Copy link
Contributor Author

yashhy commented Sep 6, 2018

Hey @yjose, along with overriding arrow left, arrow top, content left and top can also be overridden?

So with that said,

The current code for contentStyle will be:

this.ContentEl.style.top = contentStyle.top || (cords.top - helper.top + "px");
this.ContentEl.style.left = contentStyle.left || (cords.left - helper.left + "px");

and code for arrowLeft

this.ArrowEl.style.top = arrowStyle.top || cords.arrowTop;
this.ArrowEl.style.left = arrowStyle.left || cords.arrowLeft;

Let me know your thoughts.

@yjose
Copy link
Owner

yjose commented Sep 6, 2018

Hi @yashhy,
For the first one the this.ContentEl must be positioned based on the helper cords.
we use the helper to position the tooltip on the right place and we use offsetXand offsetY to customize the position, so I think no need to update the ContentEl position part.

for the arrow, I think the best way is to customize position, we can use the arrowStyle position but must be related to the current position already calculated by the calculatePosition function.

@yjose
Copy link
Owner

yjose commented Sep 13, 2018

@yashhy any update here or I can take the lead to fix it

@yashhy
Copy link
Contributor Author

yashhy commented Sep 14, 2018

@yjose sorry man, was busy with other things.

I was opening up a PR but it threw me this

Pushing to https://github.com/yjose/reactjs-popup.git
remote: Permission to yjose/reactjs-popup.git denied to yashhy.
fatal: unable to access 'https://github.com/yjose/reactjs-popup.git/': The requested URL returned error: 403

@yjose
Copy link
Owner

yjose commented Sep 14, 2018

Hi @yashhy , you need to fork the repo first ( GitHub top buttons ), then you can update and push code to your forked repo and then you can open a PR from your GitHub repo.
let me know if you need more details

yashhy pushed a commit to yashhy/reactjs-popup that referenced this issue Sep 16, 2018
…idden

 - arrowStyle.top and arrowStyle.left css property will now be respected for setting the arrow style
yjose added a commit that referenced this issue Sep 29, 2018
…left-is-been-overridden

fix issue #36 popup arrowStyle css property "left" is been overridden
@yjose yjose closed this as completed Sep 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants