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

[examples/react-rich] Fix: Invalid background-image url path cause missing Toolbar format icons in Vite build bundle. #6063

Closed
wants to merge 4 commits into from

Conversation

smartappcoder
Copy link

Description

Current vite build used by yarn build does not bundle Toolbar format icon SVG files. This causes missing Toolbar format icons in production build. This is caused by invalid background-image url path in i.undo and other Toolbar format icon selectors in 'src/styles.css'. The fix is changing those urls from

i.undo { background-image: url(icons/arrow-counterclockwise.svg); }

to set to root directory

i.undo { background-image: url(/icons/arrow-counterclockwise.svg); }

Please see more on Static Asset Handling The public Directory section.

Test Plan

  • Setup: clone the lexical repo and run the react-rich

git clone https://github.com/facebook/lexical.git
cd examples/react-rich
yarn
yarn dev

  • The example runs as expected with Toolbar format icons.

react-rich-expected

Before

  • Now run yarn build. The command will complain like this
icons/arrow-counterclockwise.svg referenced in /XXX/lexicail/examples/react-rich/src/styles.css didn't resolve at build time, it will remain unchanged to be resolved at runtime
. . .
  • yarn preview shows missing Toolbar format icons in RichText example.

react-rich-example-missing-formta-icons

  • Same errors happen to static bundle in dist.

After

  • Changes background-iamge url in src/styles.css.
  • Run yarn dev. react-rich run as expected.
  • Rerun yarn build. The command run successfully without complaints.
  • Now yarn preview and dist bundle all have expected Toolbar format icons.

react-rich-expected

…build fail to bundle Toolbar format icons.
Copy link

vercel bot commented May 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 9, 2024 4:33am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 9, 2024 4:33am

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 8, 2024
@etrepum
Copy link
Collaborator

etrepum commented May 8, 2024

I think a better fix is to move the icons folder from public to src, then the relative paths in the css will work and vite will bundle them (mostly inline as data urls)

@smartappcoder
Copy link
Author

I think a better fix is to move the icons folder from public to src, then the relative paths in the css will work and vite will bundle them (mostly inline as data urls)

I am OK to move to src/assets or even react-icons or @fontawsome. However that will impact the project structure, which IMHO might need more inputs, arbitrates, and tests.

I suggest to consider this fix as it's impact is isolated.
At the same time, I suggest open an enhancement Issue which may include data urls as you suggest, or react-icons or @fontawesome packages.

@etrepum
Copy link
Collaborator

etrepum commented May 9, 2024

If you mv public/icons src/icons it just works as-is, without changing any code

@smartappcoder
Copy link
Author

If you mv public/icons src/icons it just works as-is, without changing any code

Sure, I can do that.

…es vite build fail to bundle Toolbar format icons."

This reverts commit aa98b4c.
Move icons from /public to /src as etrepum suggests.
Copy link
Contributor

@Sahejkm Sahejkm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@StyleT StyleT left a comment

Choose a reason for hiding this comment

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

Hi!

Thanks for this PR! However I would ask you to keep images in public folder and instead simply fix URIs in css file.

Current approach makes Vite (as it's configured now) to inline all images within resulting CSS. Which is unnecessary.

See how examples/reach-rich-collab works, I fixed it there already.

@smartappcoder
Copy link
Author

Either approach should be OK as the fix is straightforward. My suggestion is to follow Lexical team's suggested static assets handling for React apps.

My two cents,

react-rich<./pre> is actually 
react-rich-with-vite
. As React suggests Next.js, Remix, Getsby, Expo etc for quickstart, Lexical may plan examples for other React frameworks as well.

See React suggested quick start frameworks Start a New React Project

@etrepum etrepum added the stale-pr PRs that are closed due to staleness. Please reopen the PR if there are new updates label Dec 2, 2024
@etrepum
Copy link
Collaborator

etrepum commented Dec 2, 2024

Unfortunately I can't merge this until the request for changes review is dismissed, if you'd like to submit this fix as a new PR please do so.

@etrepum etrepum closed this Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. stale-pr PRs that are closed due to staleness. Please reopen the PR if there are new updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants