Conversation
|
@SethFalco and team, CURRENT OUTPUT AFTER THE CHANGES: svgo % node bin/svgo.js --show-plugins | grep tyle
[ convertStyleToAttrs ] converts style to attributes
[ inlineStyles ] inline styles (additional options) (preset-default)
[ mergeStyles ] merge multiple style elements into one (preset-default)
[ minifyStyles ] minifies styles and removes unused styles (preset-default)
[ removeStyleElement ] removes <style> element |
SethFalco
left a comment
SethFalco
left a comment
There was a problem hiding this comment.
Hey hey! Thank you very much for contributing to SVGO! 👋
This looks good! I have left some minor feedback for you. If you're able to resolve them, happy to merge. If you don't have time, feel free to let us know and one of the maintainers can help take this to the finish line. 👍
|
Hi, Made the requested changes, please have a look 😄 One Thought regarding this from my side that the preset text can have a distinct formatting, maybe color, so that it is easily visible in the list of show-plugins? @SethFalco , what do you think? |
SethFalco
force-pushed
the
default-plugin-states-fix
branch
from
9bf133a to
d58a3e7
Compare
- Removed '(disabled by default)' from several plugin descriptions to prevent any discrepancy. - Improved the showAvailablePlugins function to display which presets each plugin belongs to in the plugin list output.
SethFalco
force-pushed
the
default-plugin-states-fix
branch
from
d58a3e7 to
80bcf59
Compare
There was a problem hiding this comment.
LGTM! 🎉
I did rebase for you. For future reference, I think it can be better to rebase a PR with upstream/main rather than merge upstream/main into your branch.
This keeps the git history a little cleaner, which is especially helpful during reviews, so I won't see changes unrelated to your pull request. Not everyone loves to rebase though, so up to you if you use it!
In any case, I'm happy with the implementation, and appreciate you taking the time to work on this fix.
I did make one tweak to it during my rebase, but have otherwise kept it as-is. In the original implementation, each item in the list was prefixed with a whitespace. Your PR removed this space, so I added it back.
In future, I'd like to review the formatting of the CLI, but we'll do this as a separate ticket. For this fix, I'd like to maintain as much of existing interface as possible outside of the obvious bugs.
Thank you very much for your work and patience!
|
Hi @SethFalco, Thanks for the support through the process. This was my first PR on a project I used in my own project and really happy to be able to contribute, will continue to do more in the future. Yeah, I did the rebase from upstream but I think I did it in the checked out branch instead of first doing it in the main branch of my fork. Sometimes it happens with me with git (in work too 🥲). For the future formatting for the CLI, would like to work on that too and learn and contribute more. Overall, a great thing for me as well. Thanks to the team of SVGO. |
2 participants
Closes: #2154