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

Fix to improve removal of whitespace between tables #32

Merged
merged 3 commits into from
Sep 10, 2021
Merged

Fix to improve removal of whitespace between tables #32

merged 3 commits into from
Sep 10, 2021

Conversation

TheComp44
Copy link
Contributor

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

#29 added a fix to remove whitespace nodes from tables, since they caused a warning in React (#28). Though the check for strings containing just a newline is too specific, because whitespace nodes can consist of arbitrary combinations of newlines, tabs and spaces.

When using rehype-parse and rehype-react to transform html documents containing tables that have indentation, react still gives a warning for whitespace within a table, because the check fails here.

I used a more robust check to filter out nodes containing only whitespace that will work for any combination of whitespace characters.

I also added a test to check for whitespace removal on tables.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Sep 9, 2021
@wooorm
Copy link
Member

wooorm commented Sep 9, 2021

👍

Can you use hast-util-whitespace? https://github.com/syntax-tree/hast-util-whitespace.
Because JavaScript whitespace is not the same as HTML whitespace

@codecov-commenter

This comment has been minimized.

@TheComp44
Copy link
Contributor Author

Can you use hast-util-whitespace? https://github.com/syntax-tree/hast-util-whitespace.
Because JavaScript whitespace is not the same as HTML whitespace

Probably a better solution, but then it has to be moved from the h-function as it receives elements and not HAST-nodes.

Will look into it tomorrow

@wooorm
Copy link
Member

wooorm commented Sep 9, 2021

That utility supports both strings and nodes, so it should work I think!

@TheComp44
Copy link
Contributor Author

TheComp44 commented Sep 10, 2021

Ah gotcha. Overlooked that part.

Using the utility now and it seems to work!

@wooorm wooorm changed the title Improved whitespace removal on tables Fix to improve removal of whitespace between tables Sep 10, 2021
@wooorm wooorm merged commit b97da2f into rehypejs:main Sep 10, 2021
@wooorm wooorm added the 💪 phase/solved Post is done label Sep 10, 2021
@github-actions

This comment has been minimized.

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Sep 10, 2021
@wooorm wooorm added 🐛 type/bug This is a problem 👶 semver/patch This is a backwards-compatible fix 🗄 area/interface This affects the public interface labels Sep 10, 2021
@wooorm
Copy link
Member

wooorm commented Sep 10, 2021

Thanks, released!

@wooorm wooorm mentioned this pull request Jan 18, 2023
5 tasks
wooorm pushed a commit that referenced this pull request Jan 18, 2023
Related-to GH-32.
Closes GH-45.

Reviewed-by: Titus Wormer <tituswormer@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 👶 semver/patch This is a backwards-compatible fix 🐛 type/bug This is a problem
Development

Successfully merging this pull request may close these issues.

3 participants