-
Notifications
You must be signed in to change notification settings - Fork 132
Add Nightfall Theme #1636
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
base: master
Are you sure you want to change the base?
Add Nightfall Theme #1636
Conversation
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.
Obviously, i can't review 3000 lines of code in one go, but it seems like the biggest problem right now is lots of comments, lots of commented out code, and lots of TODOs! https://blog.codinghorror.com/coding-without-comments/ and http://www.codeodor.com/index.cfm/2008/6/18/Common-Excuses-Used-To-Comment-Code-and-What-To-Do-About-Them/2293 give a more indepth explanation of why i consider these things to be problems, but the overall guiding factor here is maintainability. Having lots of unclear comments in code make it really hard to figure out what's going on two years down the line! There are three big things I think we should do here:
-
commented out code should either be uncommented or removed. There's no reason to commit into master code that doesn't work. Released code should be your best, finished production—not a "rough draft"
-
TODO comments should either be fixed or removed. In the very very rare case that there's an immediate follow-up that's too big for this pull request, it should be tracked as a github issue
-
Instead of having "section" comments, this code should be broken up into separate files using css's
@import
feature. Files should be by component or feature, and nested appropriately (maybe by dashboard, search, shared, etc?). I realize that's a lot of files, but so is 3000 lines of code! it's much much harder to find stuff when it's thrown into a file haphazardly, and i'm not looking forward to trying to fix bugs in here in the future!
My final overall comment is that you should be more liberal about using !important
when specifically overriding built-in tumblr styles. It looks like you override tumblr with selectivity a lot, but not only does this make the CSS hard to read, it's very fragile—any minor refactoring on tumblr's end can change the selectivity drastically.
As always, feel free to hit me up in discord if you need help or have questions about anything i've mentioned here!
Thanks so much for reviewing as much as you did! im pretty embarrassed that my code got this reaction, but I think it’s v fair! I thought that bc it was functional that that was enough, but knowing it’s not now I’m definitely going to restructure and fix this mess XD quick responses first tho:
Thank you again for ur comments and time! Hopefully by the time I’m finished reworking this it’ll scan a lot better and can be committed in ^__^ |
yep, that's definitely totally legit and it's why I broke down the high level goal into smaller subtasks, like splitting up the files and removing TODOs |
Ok! It's taken a while, but thankfully I'm sick and on holidays so I didn't rly have much else to do BUT this XD This should have the same end result as the first commit, but this time it should make a lot more sense as to what's happening here ^__^ Also added a few more neat graphics while I was it!
Ok! It's taken a while, but thankfully I'm sick and on holiday so I didn't rly have much else to do BUT this XD This should have the same end result as the first commit, but this time it should make a lot more sense as to what's happening here ^__^ Also added a few more neat graphics while I was it, like a fancier peepr colour and :active colours for options in most menus! |
@nightpool just pinging u in this thread ^__^ |
Ok! So Tumblr's Accessibility Update has all but broken my theme, but I put together this patch that can be deleted later once Tumblr removes their Also, cleaned up the code and tried to organise it some more. There's a lot missing (and comments present, sorry!), but I rly wanted to get the majority of the code done ASAP. |
Another theme for XKit! This one features:
Look at these cool screenshots!



Thanks to @HeyItsJono for providing a basis with their Blur theme!