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

Inline styles to fix style loading issues #298

Closed
wants to merge 8 commits into from

Conversation

thomasjm
Copy link
Contributor

This is meant to fix #121 .

The culprit was the styles in src/style/scss/global.scss, which required the use of style-loader, which doesn't work in SSR settings.

At first I tried switching it to isomorphic-style-loader, but that still felt too troublesome and heavyweight. It's such a small amount of styles that it was easiest to just inline them.

The main challenge was converting &:hover selectors to React state. I think I did this correctly, but if I missed any places it will manifest as some of the buttons appearing/not appearing when they're not supposed to with respect to hover. It would be good if someone familiar with this library could do npm run dev on this PR and click around to make sure everything still works the same.

I also upgraded a bunch of the dev dependencies like webpack. Note that css-loader/sass-loader/style-loader etc. are still present because they're used in the demo build. It would be good to inline things there too and get rid of all style loader stuff.

Thanks to @justinmchase for pinpointing the issue.

Copy link

@justinmchase justinmchase left a comment

Choose a reason for hiding this comment

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

This looks excellent!

dev-server/dist/index.html Outdated Show resolved Hide resolved
@correiamp
Copy link

@mac-s-g I've tested this PR and it works smoothly.
Do you have an ETA to go live?

@barvhaim
Copy link

waiting for this

@correiamp
Copy link

@usulpro @Kikobeats I've found in this thread that you're now contributors of this (amazing) project.
I really need this feature, could you please check if this PR can go live?

@pauljonescodes
Copy link

I'm sorry to pile in here, but I also would like this feature and have confirmed this branch works.

@paulshorey
Copy link

paulshorey commented Jan 11, 2021

Confirmed. This PR works. The unit tests actually pass. I don't have time rn to get into this Travis CI test runner misconfiguration. Anybody know how to fix it?

Working example here: https://wordio.co
npm install git://github.com/paulshorey/rjv.git

I actually cloned the code from the latest commit in this PR, and installed it in my project. Built, committed, pushed to my own repo as a quick fix.

@mac-s-g
Copy link
Owner

mac-s-g commented Jan 16, 2021

testing now

@mac-s-g
Copy link
Owner

mac-s-g commented Jan 16, 2021

migrating discussion to #311 which contains commits to this repo. it's just making it easier for me to resolve the conflicts.

thank you to everyone contributing to the code and discussion.

@mac-s-g mac-s-g closed this Jan 16, 2021
@thomasjm
Copy link
Contributor Author

Hooray, thanks @mac-s-g !

@correiamp
Copy link

Thank you @mac-s-g !

@correiamp
Copy link

@thomasjm, just to confirm, have you tested the new version with success?

I'm getting a strange problem with unit tests/code coverage and I want to confirm if it's only happening to me.
When running unit tests/code coverage, it's showing the local path of this project, like the image shows:

image

I've tried to remove it from coverage in jest.config but without success.
Do you have the same problem?

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.

ReferenceError: window is not defined
7 participants