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

Output bounding box to hover event data (new attempt) #5872

Closed
wants to merge 2 commits into from

Conversation

xhluca
Copy link

@xhluca xhluca commented Aug 2, 2021

Thanks for your interest in plotly.js!

Translations:

  • Please @ mention a few other speakers of this language who can help review your translations.
  • If you've omitted any keys from dist/translation_keys.txt - which means they will fall back on the US English text - just make a short comment about why in the PR description: the English text works fine in your language, or you would like someone else to help translating those, or whatever the reason.
  • You should only update files in lib/locales/, not those in dist/

Features, Bug fixes, and others:

Before opening a pull request, developer should:

  1. make sure they are not on the master branch of their fork as using master for a pull request would make it difficult to fetch upstream changes.
  2. fetch latest changes from upstream/master into your fork i.e. origin/master then pull origin/master from you local master.
  3. then git rebase master their local dev branch off the latest master which should be sync with upstream/master at this time.
  4. make sure to not git add the dist/ folder (the dist/ is updated only on version bumps).
  5. make sure to commit changes to the package-lock.json file (if any new dependency required).
  6. provide a title and write an overview of what the PR attempts to do with a link to the issue they are trying to address.
  7. select the Allow edits from maintainers option (see this article for more details).

After opening a pull request, developer:

  • should create a new small markdown log file using the PR number e.g. 1010_fix.md or 1010_add.md inside draftlogs folder as described in this README, commit it and push.
  • should not force push (i.e. git push -f) to remote branches associated with opened pull requests. Force pushes make it hard for maintainers to keep track of updates. Therefore, if required, please fetch upstream/master and "merge" with master instead of "rebase".

Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

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

You shouldn't commit 'npm run build'. Please drop that commit as we only commit the dist for releases. If you are interested in the build files you could simply find them in the 'publish-dist' container on CircleCi under Artifacts tab.
Thanks.

@xhluca
Copy link
Author

xhluca commented Aug 3, 2021

Thanks for the heads up. I removed the content of npm run build

// TODO: These coordinates aren't quite correct and don't take into account some offset
// I still haven't quite located (similar to xa._offset)
bbox: {
x0: pt.x0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these coordinates correct for pie?

// a sibling of the graph div, it will be positioned correctly relative to
// the offset parent, whatever that may be.
var hot = gd.offsetTop + gd.clientTop;
var hol = gd.offsetLeft + gd.clientLeft;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename hot and hol to topOffset and leftOffset.

@archmoj
Copy link
Contributor

archmoj commented Aug 23, 2021

Please update the PR description similar to #5512 and mention that PR.

@archmoj
Copy link
Contributor

archmoj commented Aug 23, 2021

Please pull master and merge origin/master into this branch, then attempt pass the tests so that all show green.

@archmoj
Copy link
Contributor

archmoj commented Aug 23, 2021

Please also provide draftlogs/5872_add.md file as mentioned on this README. Thanks!

@nicolaskruchten
Copy link
Contributor

@alexcjohnson can you help get this one over the finish line plz, even after @xhlulu leaves? :)

@archmoj archmoj modified the milestone: v2.4.0 Aug 24, 2021
@archmoj
Copy link
Contributor

archmoj commented Aug 25, 2021

No remarkable commit added when compared to the original PR #5512.
Closing.

@archmoj archmoj closed this Aug 25, 2021
@archmoj archmoj deleted the external-hover-3 branch August 25, 2021 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants