Skip to content

Conversation

SwarupKharul
Copy link
Collaborator

What does this PR do?
Added support for dracula theme

Partial fix for #1133

Tested?

  • Manually
  • Existing tests (adapted, if necessary)
  • New tests added (for any new behavior)
  • Passed linting & tests (each commit)

Commit flow

  • first commit add necessary files
  • fix typo

Notes & Questions

Visual changes
image

@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Oct 14, 2021
@SwarupKharul SwarupKharul marked this pull request as draft October 18, 2021 04:37
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@SwarupKharul This looks like a good already - thanks for investigating! 👍

I left some notes on style inline, and also while a lot of the colors appear to match, the references aren't to the colors or the dracula theme website - which maybe slightly disagree with the .vim file?

It looks like the linters should be fixable by running isort and black. The tests are failing since there is no data in the style for syntax highlighting, which the theme contribution guide may have not been updated for yet (it was added over the Summer).

-------
This theme uses the official dracula color scheme.
For color reference see:
https://github.com/dracula/vim/blob/d1864ac0734ce51150affa96a35a1e01ade08b79/colors/dracula.vim
Copy link
Collaborator

Choose a reason for hiding this comment

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

This link seems to be a file that indicates how the colors are used, rather than the actual color specification?

I found another (.vim) file in that repo that seems to (mostly) match the colors you added, but there are also colors in the official dracula theme website that seem slightly different, at least the darker ones?

The 'h' codes seem to approximate the .vim numbers; are these equivalent to the numbers in the dracula theme itself, do you know? These seem quite different, so I'm not sure if they're a different system.

# color = 16code 256code 24code
DEFAULT = 'default default default'
BG_DARK = 'dark h235 #21222C'
BG_LIGHT = 'white h253 #F8F8F2'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this FG ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

@SwarupKharul
Copy link
Collaborator Author

@SwarupKharul This looks like a good already - thanks for investigating! 👍

I left some notes on style inline, and also while a lot of the colors appear to match, the references aren't to the colors or the dracula theme website - which maybe slightly disagree with the .vim file?

It looks like the linters should be fixable by running isort and black. The tests are failing since there is no data in the style for syntax highlighting, which the theme contribution guide may have not been updated for yet (it was added over the Summer).

Thank you @neiljp! I will go through the recommended changes and do the needful.

@neiljp neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Nov 1, 2021
@SwarupKharul SwarupKharul marked this pull request as ready for review November 2, 2021 05:27
@neiljp
Copy link
Collaborator

neiljp commented Nov 17, 2021

@SwarupKharul Please let us know if you need more help :)

@SwarupKharul
Copy link
Collaborator Author

@neiljp I have made the required changes.

@neiljp neiljp added area: colors/styles/themes PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jan 30, 2022
@zulipbot
Copy link
Member

zulipbot commented Mar 4, 2022

Heads up @SwarupKharul, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

@neiljp
Copy link
Collaborator

neiljp commented Mar 5, 2022

@SwarupKharul Belated thanks for this again :)

You had failing tests, and indeed the theme is missing some elements and has some invalid ones.
Some of this may be since some code was merged between when you copied the base theme and pushing it here, such as extra required styles and code block styling (META).

In looking what was wrong, I've adjusted some tests, documentation and now have #1160 pending which should reduce some of the challenges in theme contribution.

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Mar 5, 2022
@neiljp
Copy link
Collaborator

neiljp commented Mar 30, 2022

For reference, the pygments we now depend upon should support syntax highlighting with a dracula pygments style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: colors/styles/themes has conflicts PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants