Skip to content

Added support to Ruby & VIM script languages#18

Closed
samflores wants to merge 5 commits into
PrismJS:gh-pagesfrom
samflores:gh-pages
Closed

Added support to Ruby & VIM script languages#18
samflores wants to merge 5 commits into
PrismJS:gh-pagesfrom
samflores:gh-pages

Conversation

@samflores
Copy link
Copy Markdown
Contributor

Hi,

I added a quick support to the languages I use most.

Sorry for the previous pull request. Hopefully I didn't break anything this time ;)

@LeaVerou
Copy link
Copy Markdown
Member

Hi there,

I was about to merge this right now (better late than never...) but I noticed a few things:

  • Language definitions are defined as plugins. This is not correct. They should be in the components folder, with names like prism-xxx.js and listed in code.js to be included in the custom builder.
  • It looks like some of these definitions could benefit from inheriting from the clike language. Understandably, language inheritance was not added to Prism yet when you submitted this PR. If you still want to work on this, you could look into how JavaScript, PHP and other C-style languages are doing it.
  • You have modified the prism-dark.css file, but not the other themes, which could be confusing for someone using these languages, in case they're not using that theme.

I'm really sorry it took me so long to review this. If you don't want to work on it any more, I understand. I hope it doesn't put you off future contributions. Thank you for your efforts!

I'm closing this PR to keep things tidy, but feel free to reopen if you want to work on the issues above.

@samflores
Copy link
Copy Markdown
Contributor Author

I've got some time to work on this and have one question: do you prefer one PR per language or everything in a huge PR is fine?

@LeaVerou
Copy link
Copy Markdown
Member

Yay!!

I’d prefer one PR per language. In general, the smaller PRs, the better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants