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

Numark Scratch: Manual #492

Merged
merged 9 commits into from
Jun 12, 2024
Merged

Numark Scratch: Manual #492

merged 9 commits into from
Jun 12, 2024

Conversation

NotYourAverageAl
Copy link
Contributor

mapping PR is 4834

@NotYourAverageAl
Copy link
Contributor Author

NotYourAverageAl commented Jun 30, 2022

@ronso0 help please! what's wrong with my svgs?
also the link check failure seems wrong.

@Swiftb0y
Copy link
Member

Swiftb0y commented Jul 7, 2022

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.

@NotYourAverageAl
Copy link
Contributor Author

I closed this PR by mistake. I'll have a look over the weekend. thanks

@Swiftb0y
Copy link
Member

Swiftb0y commented Jul 7, 2022

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).

@JoergAtGithub
Copy link
Member

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.
Do you've pre-commit installed locally?

@NotYourAverageAl
Copy link
Contributor Author

I created a new fresh svg and it seems local & online pre-commit worked this time around.

@ronso0
Copy link
Member

ronso0 commented Jun 20, 2023

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 ?

@NotYourAverageAl NotYourAverageAl marked this pull request as draft June 21, 2023 09:03
@ronso0
Copy link
Member

ronso0 commented Jun 21, 2023

strange, pre-commit complains again.
You can simply apply the patch from the check, lines 173-176
https://github.com/mixxxdj/manual/actions/runs/5333920658/jobs/9665031422?pr=492#step:4:173

@NotYourAverageAl
Copy link
Contributor Author

NotYourAverageAl commented Jun 21, 2023

I'll have another look when I get home. I think scour initially removed the white background when optimising. 🤷🏾‍♂️

@ronso0
Copy link
Member

ronso0 commented Jun 22, 2023

Okay, great, pre-commit is happy now!

Now please

... squash the SVG/png commits into one and force-push, if that's okay for all reviewers @JoergAtGithub ?

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

source/hardware/controllers/numark_scratch.rst Outdated Show resolved Hide resolved
source/hardware/controllers/numark_scratch.rst Outdated Show resolved Hide resolved
@JoergAtGithub
Copy link
Member

if that's okay for all reviewers @JoergAtGithub ?

I've not reviewed this PR - feel free to merge this, when you think it's ready

@ronso0
Copy link
Member

ronso0 commented Jun 22, 2023

I've started a text review, but had to leave. I hope to finish this asap

@NotYourAverageAl
Copy link
Contributor Author

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.

@JoergAtGithub
Copy link
Member

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!

@NotYourAverageAl NotYourAverageAl changed the base branch from 2.3 to 2.4 June 23, 2023 11:29
NotYourAverageAl and others added 2 commits June 23, 2023 21:31
Copy link
Member

@ronso0 ronso0 left a 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.

source/hardware/controllers/numark_scratch.rst Outdated Show resolved Hide resolved
source/hardware/controllers/numark_scratch.rst Outdated Show resolved Hide resolved
source/hardware/controllers/numark_scratch.rst Outdated Show resolved Hide resolved
@ronso0 ronso0 marked this pull request as ready for review June 24, 2023 12:43
@ronso0 ronso0 marked this pull request as draft June 24, 2023 12:47
@ronso0
Copy link
Member

ronso0 commented Jun 24, 2023

Thanks, LGTM now.

Waiting for the mapping PR

@NotYourAverageAl NotYourAverageAl marked this pull request as ready for review June 12, 2024 11:49
@Swiftb0y Swiftb0y merged commit bd4d33d into mixxxdj:2.4 Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants