-
Notifications
You must be signed in to change notification settings - Fork 37
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
Adding Vale to shared components for testing #223
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two concerns that will make this easier to review once addressed:
-
I'd prefer we not use the top-level
/styles
directory, as it can easily be mistaken for site styles. Consider something like/tool/vale/styles
. https://vale.sh/docs/topics/config/#stylespath seems to have docs on configuring it. Theini
file is fine in the root directory though. -
I don't think we should check in the styles from third-party packages. It increases the size of the repo, has licensing concerns, and it's harder to tell what styles are custom or from packages. Consider adding a
.gitignore
file within the new/styles
directory location that ignores all directories containing files not maintained by us.
Let me know if you have any concerns or issues with these changes though. Thanks!
@parlough : Agreed on moving directories. Makes sense. As for removing all styles not maintained by us, that would leave us with nothing. I want to merge this to be able to validate all of the Google styles as correct. They look to be correct with supporting links, but I want to be sure. |
Thanks for moving the directory! You'll also need to change that setting in the
If we keep them in the repo though, we would need to make sure we account for each package's license. According to Packages and VCS on the Vale docs, it seems intended to not upload them and include them in the |
@parlough : Good guidance! I'll make those changes today. |
@parlough : PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making those adjustments!
I haven't tried this out yet, but I think this should be good to land for us to start trying out. Thanks for exploring here :)
Replaces dart-lang/site-www#4231