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

Added themes for basic stars in flavors for css, fontawesome and boot… #35

Merged
merged 1 commit into from
Jun 20, 2015

Conversation

garygreen
Copy link
Contributor

Keeping it simple, just adding basic themes for the stars (most common imo). I've changed to use the unicode for basic css rather than images (can add that maybe at a later date).

Usage is just simply to wrap your rating in a container like:

<div class="br-theme-css-stars">
    <select>
      <option value="1">1</option>
      <option value="2">2</option>
      <option value="3">3</option>
      <option value="4">4</option>
      <option value="5">5</option>
    </select>
</div>

gg

@garygreen garygreen force-pushed the themes branch 2 times, most recently from 3edbc22 to d24c7f1 Compare May 27, 2015 14:02
@garygreen
Copy link
Contributor Author

One thing that might be quite nice to add in this PR is the concept of sizes. Maybe following on from bootstrap define sizes: xs, sm, md, lg and use them by adding br-size-<size>:

<div class="br-theme-basic br-stars br-size-lg">
    <select>
      <option value="1">1</option>
      <option value="2">2</option>
      <option value="3">3</option>
      <option value="4">4</option>
      <option value="5">5</option>
    </select>
</div>

Of course if you need more refinement over size then you need to do some custom css, but it would suit most people to have a fairy set of well-defined sizes.

@garygreen
Copy link
Contributor Author

Also maybe it would be better if there was theme and type option rather than doing it by manually setting a container html element. These options would set the br-theme-<theme> and br-<type> classes onto the .br-widget container element.

$('.stars').barrating({
  theme: 'bootstrap',
  type: 'stars'
});

@antennaio
Copy link
Owner

Just recently I changed how the plugin works and the wrapper div is created automatically on init (with a .br-wrapper class by default).

Passing options to the plugin sounds like a good idea - if you go for sizes as well, my vote goes for this kind of setup:

<div class="br-wrapper">
  ...hidden select field...
  <div class="br-widget br-theme-basic br-type-stars br-size-lg">
    <select>
      <option value="1">1</option>
      <option value="2">2</option>
      <option value="3">3</option>
      <option value="4">4</option>
      <option value="5">5</option>
    </select>
  </div>
</div>
$('.stars').barrating({
  theme: 'bootstrap',
  type: 'stars',
  size: 'xs'
});

In effect maybe the wrapperClass option could be removed. I think the theme option will take over its role.

@garygreen
Copy link
Contributor Author

Sounds good to me!

@garygreen
Copy link
Contributor Author

Just looking into this again, any more thoughts @antennaio ? Will push updated PR in a sec

@garygreen
Copy link
Contributor Author

@antennaio should be all good to merge now.

I've held off on the sizes, maybe those can be added as a feature in another commit. Also kept the wrapper class for backwards compatibility, maybe this can be removed in future.

Really keen on getting PR #37 merged as well, so there's automatic testing with travis on PRs and also better windows support.

@garygreen
Copy link
Contributor Author

Hmm, actually thinking maybe it might be simpler to get rid of the type option and just have it be like:

.brating({
    theme: 'bootstrap-stars'
});

Then each theme will have it's own file like boostrap-stars.less fontawesome-stars.less. Will update

@garygreen
Copy link
Contributor Author

Ok, all good to merge.

@antennaio
Copy link
Owner

Thanks @garygreen :)

antennaio added a commit that referenced this pull request Jun 20, 2015
Added themes for basic stars in flavors for css, fontawesome and boot…
@antennaio antennaio merged commit f88e50f into antennaio:master Jun 20, 2015
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