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.

Sign up for GitHub

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

Jump to bottom

Replaces star pngs w/ font awesome stars #2

Open
wants to merge 14 commits into
base: poly-rater
Choose a base branch
from
Open

Replaces star pngs w/ font awesome stars #2

wants to merge 14 commits into from

Conversation

jjinkxy
Copy link
Collaborator

@jjinkxy jjinkxy commented Jan 13, 2023 •
edited
Loading

To Do:

  • Remove StarsBuilder, CssBuilder, pngs and other commented out code
  • Delegate content_tag, concat etc to :template (aka view context)

This PR introduces the following changes:

  • Shows font awesome stars instead of the star pngs
  • Adds a new builder (StarIconBuilder)
  • Updates the style.css template
  • Adds stimulus controller template for hover effect

Example from prestage (ignore ban sign):

star_rating_vid.mp4

@jjinkxy
Add a temp new star builder using font awesome icons
d1ba880 
* Add new temporary StarsIconBuilder that sets stars as font awesome
icons
* Use temp StarsIconBuilder in ratings_for helper
* Remove generating unneeded star png and css styles using star png for
background
@jjinkxy
Fill in stars when hovering via stiumulus
085b297 
* Add stimulus controller to template
* Add data attributes needed for stimulus controller to ratings_tag and
star_icon
* Add comment that hover styling class needs to be defined
@jjinkxy jjinkxy requested a review from fschwahn January 13, 2023 08:45
@jjinkxy jjinkxy self-assigned this Jan 13, 2023
fschwahn
fschwahn approved these changes Jan 13, 2023
View reviewed changes
Copy link
Member

@fschwahn fschwahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a few minor things

lib/ajaxful_rating/stars_icon_builder.rb Outdated Show resolved Hide resolved
lib/ajaxful_rating/stars_icon_builder.rb Outdated Show resolved Hide resolved
lib/ajaxful_rating/stars_icon_builder.rb Outdated Show resolved Hide resolved
lib/generators/ajaxful_rating/ajaxful_rating_generator.rb Outdated Show resolved Hide resolved
lib/generators/ajaxful_rating/templates/star_controller.js Show resolved Hide resolved
lib/generators/ajaxful_rating/templates/style.css Outdated Show resolved Hide resolved
@fschwahn
Copy link
Member

Remove Star icon builder, css builder, pngs and other commented out code

Missed that before review, so you can disregard those comments

@jjinkxy
Remove outdated CssBuiilder, StarsBuilder, pngs & uneeded comments
6c42c66 
* Remove CssBuiilder, StarsBuilder and to references them
* Remove unneeded pngs of stars
* Remove unneeded commented out code
@jjinkxy
Remove :small option & update wrapper_dom_id method
2ac290a 
* Remove :small option since it is not used or applicable
@jjinkxy
Update star_icon to be independent of fontawesome gem & update css co…
95cb699 
…mment
@jjinkxy
Rename StarsIconBuilder to StarsBuilder
78b71ce
@jjinkxy jjinkxy requested a review from fschwahn January 17, 2023 09:58
@jjinkxy
Copy link
Collaborator Author

jjinkxy commented Jan 17, 2023

@fschwahn should I update the changelog?

fschwahn
fschwahn approved these changes Jan 17, 2023
View reviewed changes
Copy link
Member

@fschwahn fschwahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fschwahn should I update the changelog?

Ah yes, why not

fschwahn
fschwahn reviewed Jan 18, 2023
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
@jjinkxy
Fix :force_static helper option
9aafe01 
* Use @show_value from stars_builder in locale when value is not given
* Move icon_options into link_star_tag and pass to star_icon, since
those options are only needed if link is on star icon
@jjinkxy
Fix linter errors
07176b1 
* js: Missing semicolon & dangling underscore
* scss: fix properties order
@jjinkxy
Add ability to extend data-action & choose to include star controller
ad6b8d9 
* Add ability to extend data-action with other actions outside of the
gem
* Add ability to choose if star controller should be included in
wrapper or somewhere else not defined by gem
@jjinkxy
Revert "Add ability to extend data-action & choose to include star co…
64bbbb9 
…ntroller"

This reverts commit ad6b8d9.
@jjinkxy
Remove star controller & refactor icon_options to helper method
a702b6f 
* Remove star controller from ui element since controller needs to be
defined in parent element of ratings_for helper in app
* Refactor icon_options to helper method to be used outside of the gem
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fschwahn fschwahn fschwahn approved these changes

Assignees

@jjinkxy jjinkxy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jjinkxy @fschwahn