Skip to content

Conversation

motackt
Copy link

@motackt motackt commented Nov 21, 2018

Another theme for XKit! This one features:

  • CSS variable support, so it's easy to manipulate and change if anyone wants to make a theme quick! Just credit back ^__^
  • Dark posts for ease of browsing at night
  • Support for every main Tumblr page (settings excluded), not just the dashboard!
  • Fancy follow pills!
  • Fancy tags, including fun icons for the Source and Ask X a question tags
  • Some XKit support with other extensions! In time, I'll add more (hopefully!)

Look at these cool screenshots!
nightfall 1
nightfall 2
nightfall 3

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

Copy link
Member

@nightpool nightpool left a 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!

@motackt
Copy link
Author

motackt commented Nov 22, 2018

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:

  1. I read through your links, and I v much agree with them, but I’m having trouble seeing how I can apply them to css where I can’t control the selector names? Either or, for a first pass, I’m definitely going to strip away the commented out code and comments that I feel don’t describe anything (ur examples of what’s obvious definitely helps!)
  2. Will defs get on those TODO comments
  3. I... had no idea CSS could do that OTL XD I’ll start using that! And yeah, I would hate looking for bugs in this too XD
  4. I was under the impression that !important is rly bad to use in CSS, but I suppose that’s in cases where u can control the selectors in HTML. I’ll make my code more readable by stripping down selector length by removing selectors that don't rly describe what they are or contribute to the logical flow of that styling (honestly I’m thankful I don’t have to keep engaging in specificity wars)

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 ^__^

@nightpool
Copy link
Member

nightpool commented Nov 22, 2018

but I’m having trouble seeing how I can apply them to css where I can’t control the selector names

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!
@motackt
Copy link
Author

motackt commented Nov 25, 2018

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!

@motackt
Copy link
Author

motackt commented Jan 17, 2019

@nightpool just pinging u in this thread ^__^

@motackt motackt changed the title Add Nightfall Theme CSS Add Nightfall Theme Jan 21, 2019
@motackt
Copy link
Author

motackt commented Feb 3, 2019

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 body flag that's messing up my specificity rules (so named nightfall_accessibility_update_temp.css).

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.

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