-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Upgrade Dispersion plugin PNG assets to SVG #7773
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
Conversation
c877224 to
711fed1
Compare
sakertooth
left a comment
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.
Haven't tested but looks okay overall. Had a few suggestions/questions though:
|
Also wondering if any of the other plugins you've switched to SVG could use the same CSS treatment, or should they be left as SVG? |
|
Yes, several of them should be changed to pure CSS. I'll do those all together in a separate PR. |
Could you elaborate on this? |
|
I think the elaboration is that the Dispersion plugin didn't have many PNG assets; one of them was a button with text on it, so that was converted to a button with text on it, and one of them was a solid background color, so it was converted to a solid background color. The only thing that was changed into an SVG was the logo, which is the same LMMS logo SVG everything else is using. |
7af0f78 to
f217eef
Compare
f217eef to
91889cc
Compare
|
There was a conflict with #7525 and I botched my merge, now the layout is all scrambled, yuck. I'll take a look at fixing the layout later tonight. |
In this commit, the layout will still be all controls in a single row. This will change later.
@rubiefawn Has the missing icon been addressed? |
|
|
I'm choosing to go with the original horizontal-only layout
sakertooth
left a comment
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.
The effect looks fine, did a quick test, had some minor concerns but nothing really substantial.
|
While fixing the missing |
messmerd
left a comment
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.
Looks good to me code-wise






Replaces the
.pngraster assets for the native Dispersion plugin withnothing! it's all CSS now..svgvector assets.Note
Part of #7767