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

Feature request: disabled property #49

Closed
raRaRa opened this issue Oct 19, 2018 · 13 comments · Fixed by #54
Closed

Feature request: disabled property #49

raRaRa opened this issue Oct 19, 2018 · 13 comments · Fixed by #54

Comments

@raRaRa
Copy link
Contributor

raRaRa commented Oct 19, 2018

Hi.

I want to suggest a new property for <Popup /> which is disabled. I've run into an issue where I have a tooltip displayed when the mouse is hovering over a button, and when I click on the button I change the opacity of the button to 0. But unfortunately the tooltip will still show up, which is the expected behavior.

That's why I want to suggest the new prop: disabled.

When disabled is set as a prop for <Popup />, then the tooltip will be removed if it's already displayed, and it won't show up when hovering over a disabled <Popup />.

I hope this gets implemented, and I could probably help out if there's interest.

Thanks!

@yjose
Copy link
Owner

yjose commented Oct 19, 2018

HI @raRaRa , Good remark.
I suggest To open a new PR for this if you have some time of course.
The implementation would be simple I think.
lets me know if you have any problem

@raRaRa
Copy link
Contributor Author

raRaRa commented Oct 19, 2018

Great to hear! And excellent job, this is the only tooltip library that behaves and works like it should 🥇

I have a problem when I try to yarn start after yarn install. I get this error:

$ parcel serve ./stories/index.html --no-autoinstall
Server running at http://localhost:1234
×  C:\git\reactjs-popup\stories\index.js: Plugin/Preset files are not allowed to export objects, only functions. In C:\git\reactjs-popup\node_modules\babel-preset-react\lib\index.js

@yjose
Copy link
Owner

yjose commented Oct 19, 2018

let's me check

@yjose
Copy link
Owner

yjose commented Oct 19, 2018

Try to remove those deps from package.json

 "babel-preset-env": "^1.6.1",
 "babel-preset-react": "^6.24.1",

Then excute this commande rm -rf node_modules/ && yarn cache clean && yarn install and start the parcel server yarn start

@raRaRa
Copy link
Contributor Author

raRaRa commented Oct 19, 2018

Still same issue :( I also tried removing yarn.lock among the steps you mentioned.

@yjose
Copy link
Owner

yjose commented Oct 19, 2018

Just got it.
look like parcel Now, support the babel 7. so the old config tries to use the old presets.
you just need to update the the .babelrc file.

"presets": [
    "@babel/env",
    "@babel/react"
  ],

@raRaRa
Copy link
Contributor Author

raRaRa commented Oct 19, 2018

Thanks, that did the trick.

I have implemented the first version of the disabled prop here: https://github.com/raRaRa/reactjs-popup

I also created a story for it.

@yjose
Copy link
Owner

yjose commented Oct 19, 2018

cool 👍 ,

Open a PR I will review it ASAP.

@raRaRa
Copy link
Contributor Author

raRaRa commented Oct 19, 2018

Ah, will do! Are there any tests in the project where I can write a test case for it?

@yjose
Copy link
Owner

yjose commented Oct 19, 2018

Yeah of Course: https://github.com/yjose/reactjs-popup/blob/master/__test__/index.test.js

you will notice that the build from your new PR was failed, it's related to Docs App just ignore it, I am migrating to gatsbyjs over react-static.

@raRaRa
Copy link
Contributor Author

raRaRa commented Oct 20, 2018

Hi, please review my PR here: #50

@raRaRa
Copy link
Contributor Author

raRaRa commented Oct 23, 2018

Hey @yjose - any update on this? Thanks!

@yjose
Copy link
Owner

yjose commented Oct 23, 2018

Hi, this PR is in My Radar Now, I will merge it ASAP.

@yjose yjose mentioned this issue Oct 31, 2018
@yjose yjose closed this as completed in #54 Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants