riteshgandhi / ng-star-rating

Highly Customizable and Responsive Star Rating library built using Angular
MIT License
38 stars 10 forks source link

Redesigned star rating component #13

Open jpike88 opened 4 years ago

jpike88 commented 4 years ago

@riteshgandhi

https://gist.github.com/jpike88/b847b5010aac112f1d049c90fe0b3081

This has a re designed component. html and css are inlined as they're not much, inlining is better.

ZoneJS property modifications are strictly a bad idea (don't do that in your other projects...), and turning off view encapsulation is just a bad pattern unless you're really trying to do something insane.

It's not totally cleaned up, but it's a good way along the way.

The alignment bug was fixed with a hack, this is NOT guaranteed to work in all scenarios but basically the way you're calculating the half margin and half width is wrong. I've simplified them and they work, I'm not sure how/why it was done in such a weird way.

This hasn't been tested on internet explorer, but it would be simple to do a user-agent string match to tweak the half margin/widths appropriately.

I HIGHLY recommend you use TSLint/ESLint, and ensure you have 0 errors in your work, this prevents a lot of weird bugs from occurring

Enjoy

P.S not doing a PR, too busy to fork etc

riteshgandhi commented 4 years ago

@riteshgandhi

https://gist.github.com/jpike88/b847b5010aac112f1d049c90fe0b3081

This has a re designed component. html and css are inlined as they're not much, inlining is better.

ZoneJS property modifications are strictly a bad idea (don't do that in your other projects...), and turning off view encapsulation is just a bad pattern unless you're really trying to do something insane.

It's not totally cleaned up, but it's a good way along the way.

The alignment bug was fixed with a hack, this is NOT guaranteed to work in all scenarios but basically the way you're calculating the half margin and half width is wrong. I've simplified them and they work, I'm not sure how/why it was done in such a weird way.

This hasn't been tested on internet explorer, but it would be simple to do a user-agent string match to tweak the half margin/widths appropriately.

I HIGHLY recommend you use TSLint/ESLint, and ensure you have 0 errors in your work, this prevents a lot of weird bugs from occurring

Enjoy

P.S not doing a PR, too busy to fork etc

thanks Josh, I'll take a look later tonight. It was actually created for one of my assignments so thanks for fixing it.

jpike88 commented 4 years ago

For an assignment? If you’re a student, that is great work and shows you have raw talent. Just remember to be ANAL about the things that matter, nothing more. This will make your coding strong and easy to understand.

riteshgandhi commented 4 years ago

@riteshgandhi

https://gist.github.com/jpike88/b847b5010aac112f1d049c90fe0b3081

This has a re designed component. html and css are inlined as they're not much, inlining is better.

ZoneJS property modifications are strictly a bad idea (don't do that in your other projects...), and turning off view encapsulation is just a bad pattern unless you're really trying to do something insane.

It's not totally cleaned up, but it's a good way along the way.

The alignment bug was fixed with a hack, this is NOT guaranteed to work in all scenarios but basically the way you're calculating the half margin and half width is wrong. I've simplified them and they work, I'm not sure how/why it was done in such a weird way.

This hasn't been tested on internet explorer, but it would be simple to do a user-agent string match to tweak the half margin/widths appropriately.

I HIGHLY recommend you use TSLint/ESLint, and ensure you have 0 errors in your work, this prevents a lot of weird bugs from occurring

Enjoy

P.S not doing a PR, too busy to fork etc

Hey, I tested your code quickly but looks like there is some issue with half star or maybe something on my side. I will take a look later and merge.

<star-rating value="3.5" totalstars="5" checkedcolor="yellow" uncheckedcolor="black" size="24" readonly=false></star-rating>

image

jpike88 commented 4 years ago

What browser and operating system are you testing on?

riteshgandhi commented 4 years ago

What browser and operating system are you testing on?

chrome/windows 10

jpike88 commented 4 years ago

chrome/macos image:

Screen Shot 2020-02-05 at 8 54 59 PM

As you're referencing a unicode charactre, it's likely font related and how that font is rendered on different operating systems appears to differ. Will be useful to test on both OS for this.