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: Suggested Rollup config corrupts output html #6795

Closed
wants to merge 2 commits into from

Conversation

timfish
Copy link

@timfish timfish commented Aug 6, 2023

Pull Request

📖 Description

I found that the suggested Rollup config results in corrupted html:
image

In the output bundle you can see the missing space:
image

I tied these regexes and they fix the issue:
https://github.com/microsoft/fast/blob/50dba9c58b1bc6ac0c8b948f68dd0cfb6485460b/build/transform-fragments.js

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

@timfish timfish marked this pull request as ready for review August 6, 2023 09:47
@timfish
Copy link
Author

timfish commented Aug 7, 2023

There are probably still some improvements to be had with the css regex since the output still has some unnessary spaces:

image

@timfish
Copy link
Author

timfish commented Aug 9, 2023

The css regex in transform-fragments.js actually needs a minor addition. It doesn't strip leading whitespace and needs an additional ^\s+.

/(?:\s*\/\*(?:[\s\S])+?\*\/\s*)|(?:;)\s+(?=\})|\s+(?=\{)|(?<=:)\s+|\s*([{};,])\s*/g,

So it would become:

/(?:\s*\/\*(?:[\s\S])+?\*\/\s*)|(?:;)\s+(?=\})|\s+(?=\{)|(?<=:)\s+|\s*([{};,])\s*|^\s+/g

Shall I create a PR for that first?

@janechu janechu mentioned this pull request Jun 10, 2024
5 tasks
janechu added a commit that referenced this pull request Jun 11, 2024
# Pull Request

## 📖 Description

The rollup configuration docs for FAST Element v1 cause corrupted HTML, this change ports over validated configuration options used in Fluent UI web components.
 
### 🎫 Issues

Closes #6795

## ✅ Checklist

### General

<!--- Review the list and put an x in the boxes that apply. -->

- [ ] I have included a change request file using `$ yarn change`
- [ ] I have added tests for my changes.
- [ ] I have tested my changes.
- [x] I have updated the project documentation to reflect my changes.
- [x] I have read the [CONTRIBUTING](https://github.com/microsoft/fast/blob/master/CONTRIBUTING.md) documentation and followed the [standards](https://github.com/microsoft/fast/blob/master/CODE_OF_CONDUCT.md#our-standards) for this project.
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.

2 participants