-
Notifications
You must be signed in to change notification settings - Fork 124
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
Numark Scratch: Manual #492
Conversation
@ronso0 help please! what's wrong with my svgs? |
I'm sorry, the scour hook is weird sometimes. Just run it manually on the SVG it complains about and then keep amending the last commit until it passes on CI. |
I closed this PR by mistake. I'll have a look over the weekend. thanks |
No problem, I figured that. I just wanted to say that you don't really have to worry about the scour hook. I'm looking into switching from scour to svgo which does not seem to have this "instability" (also its still maintained while scour isn't). |
I see that you had problems to get the SVG images through pre-commit. But changing the format from SVG to PNG is not the right approach. |
I created a new fresh svg and it seems local & online pre-commit worked this time around. |
Please add a white background to SVGs to ensure it's readable even if a browser uses different main background color (dark/night mode or other reason). IMO you can then squash the SVG/png commits into one and force-push, if that's okay for all reviewers @JoergAtGithub ? |
strange, pre-commit complains again. |
I'll have another look when I get home. I think scour initially removed the white background when optimising. 🤷🏾♂️ |
Okay, great, pre-commit is happy now! Now please
|
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.
lgtm otherwise
I've not reviewed this PR - feel free to merge this, when you think it's ready |
I've started a text review, but had to leave. I hope to finish this asap |
Please note I'll need to rebase this manual to 2.4 as the mapping is based on the 2.4 branch. Or the rebase the mapping back to 2.3 to get it out quicker. |
We updated the JavaScript engine from 2.3 to 2.4. Therefore you would need to change the syntax at various places to backport it to 2.3. Therefore better keep the mapping on target 2.4 and rebase the manual! |
Co-authored-by: Swiftb0y <[email protected]>
Co-authored-by: Swiftb0y <[email protected]>
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.
Some ideas to improve the flow and reduce redundancy.
Thanks, LGTM now. Waiting for the mapping PR |
mapping PR is 4834