fix(PF4): tooltip/popover - include tippy css #1244
Conversation
jschuler
requested review from
dlabaj,
dmiller9911,
ibolton336,
kmcfaul and
tlabaj
|
PatternFly-React preview: https://1244-pr-patternfly-react-patternfly.surge.sh |
| import PopoverHeader from './PopoverHeader'; | ||
| import PopoverCloseButton from './PopoverCloseButton'; | ||
| import GenerateId from '../../internal/GenerateId/GenerateId'; | ||
| import 'tippy.js/dist/tippy.css'; |
There was a problem hiding this comment.
This comes back to requiring users to load CSS and forces them to set up min-css-loader, styled-loader, mini-css-plugin. As for a way around it could do
- Use something other than Tippy. Maybe check to see what Material-UI does. They may have written their own though.
- Modify the babel plugin to support loading external CSS and inject it.
- Write a script to generate a file that loads the CSS and then uses injectGlobal to inject it.
- Leave it and require consumers to use webpack, css loaders, etc.
There was a problem hiding this comment.
I could go with 3, use raw-loader to get the string and then use emotion injectGlobal to inject it
There was a problem hiding this comment.
Just thought of this, we already recommend they import this to get the base css, wouldn't this fall under the same issues you listed?
import '@patternfly/react-core/dist/styles/base.css';
There was a problem hiding this comment.
For the base css that is not the only way to load it. It is also possible to copy it to a public direct, etc. Either way the user has to explicitly import it so it is assumed they know how to set that up. Instead of us assuming they have a good build in place. As for raw loader. It is the same deal. We would assume they have raw loader setup.
I had in the future planned to make a provider that added the base css for the user, but I admittly have decided to not contribute anymore code. Unless something I wrote needs fixed.
I am ok with any option you go with I just wanted you to be aware that this could potentially be a breaking change and will need documentated well
There was a problem hiding this comment.
I believe I was hitting the same problem in #1250 while trying to include an example CSS file...
I ended up just using gatsby-plugin-sass with iso-morphic-style-loader for the time being:
priley86@219c3ac
I plan to drop this commit unless we determine it useful for others. FWIW, there is a really good list of supported CSS options in Gatsby here:
https://www.gatsbyjs.org/tutorial/part-two/#component-css
Would be really nice to see gatsby-plugin-sass and gatsby-plugin-emotion support in PF-React. To @dmiller9911's point, this may put some impetus on the consumer to use a webpack loader, so it might be worth asking around first...
There was a problem hiding this comment.
I've just copied the source for now and am injecting it along with my overrides using emotion
There was a problem hiding this comment.
yea i was having trouble w/ the plugins mentioned above. This may be the easiest route for now...
Pull Request Test Coverage Report for Build 4280
💛 - Coveralls |
tlabaj
left a comment
tlabaj
left a comment
There was a problem hiding this comment.
LGTM. Just needs conflicts resolved.
| import { StyleSheet } from '@patternfly/react-styles'; | ||
|
|
||
| // inject the base tippy styles ('tippy.js/dist/tippy.css') | ||
| export const tippyStyles = StyleSheet.parse( |
There was a problem hiding this comment.
This can just use injectGlobal it will be faster since parse runs some regexs on the string.
|
|
||
| // Need to unset tippy default styles | ||
| // Also for enableFlip, need to make arrow aware of parent x-placement attribute in order to work | ||
| export const overrides = StyleSheet.parse(` |
There was a problem hiding this comment.
injectGlobal can be used here too.
* fix(PF4): tooltip/popover - include tippy css * add a jest transform ignore pattern for tippy * emotion for styling * rebase * ts * ts definitions * snapshot * prod build
8 participants
Closes: #1189
Is there a better way? Guess this css is globally added for consumers