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

feat: add manual inject styles function into the project #1041

Merged
merged 11 commits into from
Jun 27, 2023

Conversation

danielbarion
Copy link
Member

idea from #1025

@danielbarion
Copy link
Member Author

I created the flow to inject the styles manually from the package instead of letting Rollup handle it by itself.

what is pending?

  • add a switch to enable and disable styles injection.

Suggestion:
In the current way that this was implemented, we can add the switch by environment variables, ex:
REACT_TOOLTIP_DISABLE_STYLES=true

But this doesn't sound like the best way to handle this switch.

So, I need help here :)

Comment on lines 38 to 78
function removeStyle() {
const style = document.getElementById(REACT_TOOLTIP_STYLES_ID)
if (!style) {
return
}
style.remove()
}
Copy link
Member

Choose a reason for hiding this comment

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

Please test if something like this works (too lazy to setup and test it myself 😑)

The idea is to use import { removeStyle } from 'react-tooltip'

src/index.tsx Outdated Show resolved Hide resolved
@gabrieljablonski
Copy link
Member

How does removing the style injection affect the tooltip functionally? Since it depends on the .tooltip and .show classes to work properly, will the tooltip still show and hide without any issues?

@danielbarion
Copy link
Member Author

How does removing the style injection affect the tooltip functionally? Since it depends on the .tooltip and .show classes to work properly, will the tooltip still show and hide without any issues?

This will probably break the tooltip feature, one thing that we can do is separate it into 2 CSS files:

  1. for basic functionality - keep the tooltip working as expected
  2. for styling - without this one, we will only show and hide the tooltip content without any styling

please let me know your thoughts about it

@danielbarion
Copy link
Member Author

@gabrieljablonski confirming that this works but breaks the tooltip, please let me know your thoughts about my previous message

@gabrieljablonski
Copy link
Member

@danielbarion Having a separate injection for functional styling seems like an adequate solution.

My suggestion (make sure to check if I missed something):

/* react-tooltip-core.css (feel free to choose a better name for the file) */
.tooltip {
  position: absolute;
  visibility: hidden;
  opacity: 0;
}

.fixed {
  position: fixed;
}

.arrow {
  position: absolute;
}

.noArrow {
  display: none;
}

.show {
  visibility: visible;
  opacity: var(--rt-opacity);
}

.fixed and .noArrow could be left out. I included them just so the positionStrategy and noArrow props still work. Your call if we should consider them functional styling or not.

@danielbarion
Copy link
Member Author

Beta version released with the last commit:

yarn add react-tooltip@5.15.0-beta.1041.0

or

npm install react-tooltip@5.15.0-beta.1041.0

I only tested without the styles, I'll test everything later, but feel free to test it or update anything.

Thanks!

Comment on lines +582 to +606
coreStyles['arrow'],
styles['arrow'],
Copy link
Member

Choose a reason for hiding this comment

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

Is keeping both styles intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but please feel free to reorganize the styles, I tried to keep the necessary CSS for position and basic behavior into the Core file, I'll check again this week when I have more time, but please, go ahead if you want to change anything

@danielbarion
Copy link
Member Author

danielbarion commented Jun 16, 2023 via email

@danielbarion
Copy link
Member Author

danielbarion commented Jun 19, 2023

Screen.Recording.2023-06-18.at.23.08.17.mov

@gabrieljablonski 🔝

The code can be used in a button or inside of a useEffect:

useEffect(() => {
  removeStyle()
}, [])

@gabrieljablonski
Copy link
Member

The code can be used in a button or inside of a useEffect

Does calling it globally (outside any components) work? That would be the most convenient IMO

@danielbarion
Copy link
Member Author

@gabrieljablonski I'm not totally sure that I got how to test it on Next 13, but it breaks because I'm importing from a custom export:

"use client"

export * from 'react-tooltip'

and importing this function from the package also breaks it too.

Can you test it or guide me on how to test it, please?

My test environment is just a fresh Next 13 project and I'm using Yalc to test the implementation, but you can try the beta release from NPM instead of Yalc

@danielbarion
Copy link
Member Author

danielbarion commented Jun 19, 2023

@gabrieljablonski another option is:

  • let the dev import the styles (basically the colors - content from styles.module.css) and we only inject the core styles by default

@gabrieljablonski
Copy link
Member

gabrieljablonski commented Jun 19, 2023

@gabrieljablonski another option is:

  • let the dev import the styles (basically the colors - content from styles.module.css) and we only inject the core styles by default

If we do this we're basically back to how it was before, which would not be ideal IMO.


What I meant with the removeStyle() was to do something like

import { removeStyle } from 'react-tooltip'

removeStyle()

function MyComponent() {
  ...
}

This way, there's no need to put it inside a useEffect(). I did some quick testing and this seems to work.

As for doing this with the "use client" snippet, I'm not sure what issues you might've had. On a fresh Next.js 13 project I've created, I couldn't get the snippet to work because of this error https://nextjs.org/docs/messages/export-all-in-page. We might have to update the snippet to something like

"use client"

export { Tooltip } from 'react-tooltip'

Either way, users should be fine to create the "use client" snippet, while importing the removeStyle() function directly from the react-tooltip package, and calling it on their App or index file.

@danielbarion
Copy link
Member Author

danielbarion commented Jun 19, 2023

If we do this we're back to how it was before, which would not be ideal IMO.

I do agree, but now we have the basic feature of the Tooltip with the Core styles, before nothing was working because of 0 styles of tooltip on the page. I'm just thinking of projects that don't want to inject the colors from react-tooltip and this can cause re-paint of the elements in the page (probably this impact will be minimal, but I'm thinking if it's possible to reduce it to 0).

Does it make sense?

edit: maybe adding an option using environment variable to prevent the colors being injected and if no environment variable set, always inject all the styles (and keep the option to use the removeStyle() can be a good approach

@gabrieljablonski
Copy link
Member

Since most users will probably just use the default styling and partially override the styling, I really like the idea of not having to import something to apply the styles.

I'm fine with the env variable approach, but then we should consider how useful still having removeStyle() would be. Would anyone really want to remove the default styling on the fly? So having just the one way to do it feels a lot cleaner.

@danielbarion
Copy link
Member Author

Keeping the removeStyles is great for simple projects using pure HTML (importing React from CDN), or projects using other languages on the server side (like Laravel projects or something like that).

I don't know, I'm probably overthinking here, if you believe it's not useful to keep the remove styles function, I'll agree with you and all good :)

@gabrieljablonski
Copy link
Member

Keeping the removeStyles is great for simple projects using pure HTML (importing React from CDN), or projects using other languages on the server side (like Laravel projects or something like that).

Hadn't thought of these use cases, so yeah, keeping both methods should be fine then.

Though I'll suggest using the env variable should be the preferred method suggested in the docs.

@danielbarion danielbarion force-pushed the feat/add-manual-inject-styles-flow branch from ec1c3d9 to 5379e94 Compare June 23, 2023 22:05
@danielbarion danielbarion marked this pull request as ready for review June 23, 2023 22:06
@danielbarion
Copy link
Member Author

@gabrieljablonski the force push was because I did git pull -r origin master, after the rebase, I had to push -f

@danielbarion
Copy link
Member Author

yarn add react-tooltip@5.16.0-beta.1041.0

or

npm install react-tooltip@5.16.0-beta.1041.0

Check the PR changes for more information.

@dsternlicht
Copy link

@danielbarion any idea when this is going to be merged officially? :)

@gabrieljablonski
Copy link
Member

@dsternlicht just working out the final kinks. An official version should be released in a couple days.

Until then, don't worry about using the beta version, it should work just fine.

@dsternlicht
Copy link

@dsternlicht just working out the final kinks. An official version should be released in a couple days.

Until then, don't worry about using the beta version, it should work just fine.

Sounds good. Thanks @gabrieljablonski!

@danielbarion
Copy link
Member Author

@gabrieljablonski I tested again and I did two new small commits, please check them before we merge this PR

thanks!

@dsternlicht
Copy link

🎉

@danielbarion danielbarion force-pushed the feat/add-manual-inject-styles-flow branch from 40d404d to 424ef70 Compare June 27, 2023 13:26
@danielbarion danielbarion merged commit 3a1ed77 into master Jun 27, 2023
5 checks passed
@danielbarion danielbarion deleted the feat/add-manual-inject-styles-flow branch June 27, 2023 13:44
@danielbarion
Copy link
Member Author

available at v5.16.0

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants