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

Feat/additional themes #540

Merged
merged 15 commits into from
Nov 6, 2020
Merged

Conversation

tarnung
Copy link
Collaborator

@tarnung tarnung commented Oct 26, 2020

@tarnung
Copy link
Collaborator Author

tarnung commented Oct 26, 2020

code light and smyck light are not usable right now. I have to tune highlighting and maybe some colors

@tarnung
Copy link
Collaborator Author

tarnung commented Oct 26, 2020

now we're good.

@munen
Copy link
Collaborator

munen commented Oct 28, 2020

An initial visual check of the themes shows that they all look amazing! 💯

There is a little bug if there's no theme selected, yet:

The danger of checking against undefined continues(;

image

image

Of course, we can fix the issue inline in this PR. But my hunch in #534 (comment) that the lack of a default colorScheme will cause more problems later on is confirmed. So, it would be best if both (default and a more robust check) can be tackled.

@munen
Copy link
Collaborator

munen commented Oct 28, 2020

I looked into it a bit more. It was a good hunch at #534 (comment), indeed. loadTheme is called with the actual arguments "null" (as a string), because there's no default.

@tarnung
Copy link
Collaborator Author

tarnung commented Oct 28, 2020

I thought I had covered all the bases by checking against undefined AND "undefined" (as a string).
But actually having a default value would be better, I agree. I'll look into it later this week.

Something else that just came to mind:
I left the colors.css file intact to give the variables an initial value. This would be no longer necessary technically, since we set all the variables in code (once this reliably works). But I guess there's no harm in keeping it (apart from the duplication).

@tarnung
Copy link
Collaborator Author

tarnung commented Nov 2, 2020

I rebased, removed the check for 'undefined' and added a default value for theme.

@munen munen force-pushed the feat/additional-themes branch from 27c1771 to e5fd039 Compare November 6, 2020 13:00
@munen
Copy link
Collaborator

munen commented Nov 6, 2020

@tarnung Really good work! I've added contribution to the changelog and added a section to the documentation, because this is too pretty to leave it off the docs(;

image

@munen munen merged commit b3c2676 into 200ok-ch:master Nov 6, 2020
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