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

Allows for scalability and adding more networks #16

Merged
merged 7 commits into from
Nov 13, 2018
Merged

Allows for scalability and adding more networks #16

merged 7 commits into from
Nov 13, 2018

Conversation

danpastori
Copy link
Contributor

This is by far the biggest change. I'd merge this last after #13, #14, & #15 (sorry, first time trying to do a bunch of dependent PRs!).

First Change

The first thing I changed was in the initial setup function. The first parameter was 'elements' and the second was options. I removed the elements parameter. Reason being was that we never passed the elements in, we grabbed the elements from the config.selector. The second value options was never being merged into the config so users couldn't set their options. It was always null.

Second Change

Next I made the format of the config.selector to be the name-of-selector instead of [name-of-selector] essentially removing the brackets. The reason for this is we can allow the user to override and we can reference this selector when we grab the elements in the setup that will become Shr objects. We can also use this to get the link attribute (which was hard coded before so if the user overwrote this, it'd be different).

Third Change

This is one of the bigger changes but allows for scalability and closes #11 . I divided the defaults into two objects, settings and defaults. The settings are settings that the user can NOT override! With everything in one object, when we run the extend, they could accidentally override valuable settings. The defaults are pieces of data the user CAN override. In the setup method, I merge the user defaults first, then the settings. This way the settings will always overwrite any accidental user configs that would end up breaking Shr. The settings are things the user should never overwrite anyway.

Fourth Change

Within the settings, I added the keys for each network. Each network has two methods url() and shareCount(). The url() method builds the url for the network and the shareCount() returns the piece of data from JSON to get the number we are looking for. This solves issue #7 by getting rid of the configuration values and makes one single location to edit any changes. It also makes essentially an interface for adding more networks as you can just add a key with those methods and they will return the proper data.

Fifth Change

Like the settings, I added keys for each network in the defaults. This will open up an API for the user to set specific settings for each network. If this gets approved and we continue on to #12, this is where we can have default values for the youtube_subscribe network or any other network we add. Right now, the pop up variables are in there and I can submit documentation on overriding the width and height. When everything gets merged together, the settings will just add the proper methods to these keys for each network into one pretty config.

Sixth Change

The on method for each Shr element passed increment to the getCount() method. I removed that and sent it to the displayCount() method. The getCount() method doesn't account for the flag. I also based it off of the config where we have a setting for if we increment or not.

This PR will close #7 & #11

If this gets approved and merged, I'll add YouTube subscribe (#12) and update the documentation (#10). Those are dependent upon this new structure being the way to configure Shr.

…hr-button` as the main object. This will allow shr to be used in modern frameworks.
…o return the share count. Also divided settings from user defaults. This ensures users don't overwrite options they don't mean to. Fixes #7 and #11.
@sampotts
Copy link
Owner

Hey, it looks like all the PRs contain the same changes unless I'm missing something? Maybe we can just work out of one PR for now (whichever is latest) and I'll review in one go?

@sampotts sampotts merged commit f2cff0e into sampotts:master Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants