Skip to content
This repository has been archived by the owner on Jun 13, 2020. It is now read-only.

Added option to make expanded images fit to screen, ran inline-expanding.js through jshint #99

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Sep 27, 2014

Adds an option in general options tab to make inline expanded images to fit to your screen like 4chan's 'fit expanded images to screen' option

@czaks
Copy link
Collaborator

czaks commented Sep 27, 2014

same problem as with all your scripts. you pollute the global namespace when not needed. incorrect usage of options api. use of names like $mrCheckie instead of some descriptive ones.

@ctrlcctrlv
Copy link
Owner

@czaks been having same problem with @anonfagola too, I had to rewrite his scripts today (id_colors.js, id_highlight.js) because of this

We should probably push some code quality guidelines into here and vichan-devel/vichan given all the new help we're getting

@anonfagola
Copy link
Contributor

@ctrlcctrlv :(
but yeah i agree 100%

@czaks
Copy link
Collaborator

czaks commented Sep 27, 2014

it's hard to write all code quality guidelines to a file. that's why we have code review. people should learn by reading existing code.

with just javascript, it's not that hard and stability is not critical. just note on how hard it is to submit a patch to linux kernel for instance :)

@czaks
Copy link
Collaborator

czaks commented Sep 27, 2014

btw. https://github.com/ctrlcctrlv/8chan/pull/20/files this code is really clean, @anonfagola .

a code should be short, concise, just like this one. and in my opinion, we shouldn't fix pull requests ourselves, but tell our committers, what is wrong. this way they would learn.

@ghost
Copy link
Author

ghost commented Oct 2, 2014

Uses options API now, $mrCheckie variable removed. Global variables removed hopefully.

@aeosynth
Copy link
Contributor

aeosynth commented Oct 4, 2014

re code quality, we could use a linter, such as jshint

…olons,added check to see if ctrl key is down with event.ctrlKey boolean when clicking on image
@ghost ghost changed the title Added option to make expanded images fit to screen Added option to make expanded images fit to screen, ran inline-expanding.js through jshint Oct 4, 2014
@czaks
Copy link
Collaborator

czaks commented Oct 6, 2014

this.childNodes[0] keeps being repeated in the code.

there are a few indentation problems

if (window.Options && Options.get_tab("general")){ forbids use of the script without Options enabled

+ Options.extend_tab('general', '<div><label id=\"toggle-image-fittoscreen\"><input type=\"checkbox\"> fit expanded images to screen.</label></div>');

make it:

+ Options.extend_tab('general', '<div><label id=\"toggle-image-fittoscreen\"><input type=\"checkbox\"> '+_("Fit expanded images to screen.")+'</label></div>');

in order to make it l10nable

var isSpoiler = (fileInfo.indexOf("Spoiler") > -1) ? true:false;

you can shorten it to:

var isSpoiler = fileInfo.indexOf("Spoiler") > -1;

…ble. changed how max browser height is calculated to include fixed boardlist
@ghost
Copy link
Author

ghost commented Oct 9, 2014

Thanks czaks I've followed your advice. I've also attempted to fix my indentation issues as well.
Instead of calling 'this.childNodes[0]' I've declared a variable called childNode which is assigned the value of 'this.childNodes[0]' I don't know if this has fixed the repeating issue you've mentioned.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants