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

Adds four new extensions #1653

Merged
merged 6 commits into from Mar 13, 2019

Conversation

Projects
None yet
2 participants
@9999years
Copy link

9999years commented Feb 3, 2019

Adds ports of my Chrome extensions for Tumblr as XKit extensions. These include SHARP (sharp_corners), Maxwidth (dont_stretch_photosets), Transparent (transparent_img_hover), and Tumblr Titles (titles).

Now the important stuff: the icons.
dont_stretch_photosets:
dont_stretch_photosets icon
sharp_corners:
sharp_corners icon
transparent_img_hover:
transparent_img_hover icon
titles:
titles icon

I’m not really attached to the icons and don’t mind changing them.

9999years added some commits Feb 3, 2019

Adds four new extensions
Adds ports of my Chrome extensions for Tumblr as XKit extensions. These
include SHARP (sharp_corners), Maxwidth (dont_stretch_photosets),
Transparent (transparent_img_hover), and Tumblr Titles (titles).

SHARP: https://chrome.google.com/webstore/detail/sharp/adghfmelkopiojohfdlfaphobhjjahjh
MAXWIDTH: https://chrome.google.com/webstore/detail/maxwidth/pibfemlbaejpbfhipffkdljhnldadcba
Transparent: https://chrome.google.com/webstore/detail/transparent/phkhgilamababakkmpaegfafbefhgfoe
Tumblr Titles: https://chrome.google.com/webstore/detail/tumblr-titles/dflmgglllgebfghgjdgnmdhpebbdkejj
Fixes extension typos, adds XKit.interface.post_window.reblogging_from
- Fixed typos resulting from copy/pasted extension code...
  - All four extensions were writing to XKit.extensions.max_img_width...
- Cleaned up the code for `titles` to be a bit more XKit-ey, including
  fixing a non-working XKit function and adding a new one

@9999years 9999years force-pushed the 9999years:master branch from 01f5520 to 9ca56d0 Feb 3, 2019

@nightpool

This comment has been minimized.

Copy link
Member

nightpool commented Feb 3, 2019

hey, these extensions look good, but we can't edit xkit.js without releasing an entirely new add-on version. please make the changes in xkit_patches.js instead! thanks~

@9999years

This comment has been minimized.

Copy link
Author

9999years commented Feb 3, 2019

OK, moved the patches to xkit_patches.js and reverted xkit.js back to its original state.

@nightpool
Copy link
Member

nightpool left a comment

Hey! thanks for contributing these, that means a lot! I promise we'll take good care of them :D

I left some code style and readability comments—as you can imagine, maintainability is a huge, huge goal for us here. We try to make our code as readable as possible, and have it convey its intent as well as its function. This is really crucial as we often end up having to maintain this code for a very long time! I hope this doesn't seem like too many changes—feel free to disagree or let me know if you're confused about a suggestion I made, I'm always happy to talk through things over discord/etc!

a couple high level notes:

  • I'm not sure I get the point of the trimPipe function—isn't it going to be overwritten by getTitle a couple lines later?
  • ideally, it would be nice of the remove function restores the title to the way it was before we ran the script. This is kind of optional, since the title is one of the least noticeable things, but it would make for a very nice touch! this can easily be done by storing the old title in a variable the first time you run the script, in the run function. let me know if that makes sense!
Show resolved Hide resolved Extensions/xkit_patches.js Outdated
Show resolved Hide resolved Extensions/xkit_patches.js Outdated
Show resolved Hide resolved Extensions/xkit_patches.js Outdated
Show resolved Hide resolved Extensions/titles.js Outdated
Show resolved Hide resolved Extensions/titles.js Outdated
Show resolved Hide resolved Extensions/titles.js Outdated
Show resolved Hide resolved Extensions/titles.js Outdated
Show resolved Hide resolved Extensions/titles.js Outdated
Show resolved Hide resolved Extensions/titles.js Outdated
Show resolved Hide resolved Extensions/titles.js Outdated
@9999years
Copy link
Author

9999years left a comment

  • Removed hashchange event listener (not sure what functionality it was supposed to serve, but short of polling there’s no way to capture changes to the URL outside of the hash portion.
  • Renamed “URL” to “path” in a bunch of places
  • Cleaned up jQuery selectors, added template literals
  • Replaced some functions with simpler implementations as suggested
@9999years

This comment has been minimized.

Copy link
Author

9999years commented Feb 6, 2019

BTW: Are the extension names OK? I tried to lean towards descriptive but they ended up a little more verbose than I’d prefer (dont_stretch_photosets).

@nightpool

This comment has been minimized.

Copy link
Member

nightpool commented Feb 6, 2019

@nightpool

This comment has been minimized.

Copy link
Member

nightpool commented Mar 13, 2019

changes look good to me! sorry for the delay. Made one very small style fix. Will merge when builds pass!

@nightpool nightpool merged commit 69b73e1 into new-xkit:master Mar 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.