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: update twind example #178

Closed
wants to merge 0 commits into from

Conversation

Palbahngmiyine
Copy link

Existing twind examples use the deprecated twind library.
So this pr is update example to @twind/core instead of the no longer maintained twind build.

The updated example was modified by referring to the example page in twind repository.

twind/app/entry.client.tsx Outdated Show resolved Hide resolved
twind/app/entry.server.tsx Outdated Show resolved Hide resolved
@Palbahngmiyine
Copy link
Author

Palbahngmiyine commented Mar 21, 2023

Hello, @MichaelDeBoey! i I checked the adjustments and update it! However, in the current twind example, the actual example does not work due to a bug caused by hydration mismatch.

This seems to have been resolved in the next react version(18.3.0), and when tested with the actual react@next, react-dom@next version, i found that it actually worked.

Therefore, I think it would be good to freeze this PR and merge it when the react version is updated.

@MichaelDeBoey MichaelDeBoey marked this pull request as draft March 21, 2023 18:06
@MichaelDeBoey
Copy link
Member

MichaelDeBoey commented Mar 21, 2023

@Palbahngmiyine Did it work with current React version without my suggestions (so original PR)?

@Palbahngmiyine
Copy link
Author

@MichaelDeBoey

It was my mistake, the original PR(without suggestions) was also found to be not working, I can change it to work with a slight fix (entry.client.tsx, entry.server.tsx needed) though.

Apart from that, existing examples(current main branch) do not work properly, so I suggest a direction to open and modify a new PR with a working example implementation that adds entry.client.tsx, entry.server.tsx instead of the current PR.

If you agree with the suggestion, I'll be able to work immediately, and below is the error stacktrace of the existing example(current main branch).

stacktrace

@MichaelDeBoey
Copy link
Member

@Palbahngmiyine You can (force) push your changes in this PR as well if you want

@Palbahngmiyine
Copy link
Author

@MichaelDeBoey

Thank you for your kind comment, and as I mentioned before, I fixed an issue where the existing example didn't work correctly.

When the react version is updated, I'll create a new PR reflecting what is mentioned in #159 .

Thank you!

@Palbahngmiyine Palbahngmiyine marked this pull request as ready for review March 25, 2023 02:39
@MichaelDeBoey
Copy link
Member

Hi @Palbahngmiyine!

Are you still interested in getting this one merged?

If so, please rebase onto latest main & fix conflicts

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Jun 6, 2023
@Palbahngmiyine
Copy link
Author

Hello, @MichaelDeBoey !

I hope this PR will be merged and I fixed the conflict with the main branch 😊.

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Jun 7, 2023
Comment on lines 10 to 26
const hydrate = () =>
startTransition(() => {
hydrateRoot(
document,
<StrictMode>
<RemixBrowser />
</StrictMode>
);
});

if (typeof requestIdleCallback == "function") {
requestIdleCallback(hydrate);
} else {
// Safari doesn't support requestIdleCallback
// https://caniuse.com/requestidlecallback
setTimeout(hydrate, 1);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const hydrate = () =>
startTransition(() => {
hydrateRoot(
document,
<StrictMode>
<RemixBrowser />
</StrictMode>
);
});
if (typeof requestIdleCallback == "function") {
requestIdleCallback(hydrate);
} else {
// Safari doesn't support requestIdleCallback
// https://caniuse.com/requestidlecallback
setTimeout(hydrate, 1);
}
startTransition(() => {
hydrateRoot(
document,
<StrictMode>
<RemixBrowser />
</StrictMode>
);
});

Comment on lines 6 to 8
import config from "../twind.config";

install(config);
Copy link
Member

Choose a reason for hiding this comment

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

Please put this in root.tsx again

Comment on lines 12 to 32
const handleRequest = (
request: Request,
responseStatusCode: number,
responseHeaders: Headers,
remixContext: EntryContext
) =>
isbot(request.headers.get("user-agent"))
? handleBotRequest(
request,
responseStatusCode,
responseHeaders,
remixContext
)
: handleBrowserRequest(
request,
responseStatusCode,
responseHeaders,
remixContext
);
export default handleRequest;
Copy link
Member

Choose a reason for hiding this comment

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

Please keep this as it was and only update the actual needed changes

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Jun 7, 2023
@Palbahngmiyine Palbahngmiyine force-pushed the update-twind branch 2 times, most recently from 73b6d67 to 901acf0 Compare June 23, 2023 06:34
@Palbahngmiyine
Copy link
Author

Palbahngmiyine commented Jun 23, 2023

I'm Sorry, I closed PR due to IDE settings while resolving the latest main branch conflict. I will revise it again to reflect your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-response We need a response from the original author about this issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants