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

Add initial workshop project click implementation #388

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@MaxWofford
Member

MaxWofford commented Dec 5, 2018

No description provided.

MaxWofford added some commits Dec 5, 2018

@lachlanjc

This comment has been minimized.

Member

lachlanjc commented Dec 5, 2018

Thank you!! 💖

@zachlatta

Looks good to me for the most part. Left a few comments that need to be addressed before merge.

Show resolved Hide resolved app/controllers/v1/workshop_project_clicks_controller.rb
Show resolved Hide resolved app/models/workshop_project_click.rb
Show resolved Hide resolved app/models/workshop_project_click.rb Outdated
Show resolved Hide resolved spec/factories/workshop_project_clicks.rb
Show resolved Hide resolved spec/models/workshop_project_click_spec.rb Outdated
Show resolved Hide resolved spec/models/workshop_project_click_spec.rb Outdated
@zachlatta

Sorry just one last comment I forgot in earlier review.

Show resolved Hide resolved app/serializers/workshop_project_click_serializer.rb Outdated
@zachlatta

This comment has been minimized.

Member

zachlatta commented Dec 5, 2018

Oh, and on the above comment, do you intend to render the click count anywhere?

@MaxWofford

This comment has been minimized.

Member

MaxWofford commented Dec 5, 2018

@zachlatta Just addressed each of the comments with the new changes

@MaxWofford

This comment has been minimized.

Member

MaxWofford commented Dec 5, 2018

re: click count

Will add it once I talk to @lachlanjc & @polytroper about where they want the info displayed

@lachlanjc

This comment has been minimized.

Member

lachlanjc commented on app/models/workshop_project_click.rb in b0a9b4a Dec 5, 2018

Which of these is preferable, for my future reference?

This comment has been minimized.

Member

zachlatta replied Dec 5, 2018

Whichever feels more intuitive in the moment. No need for hard and fast rules.

This comment has been minimized.

Member

MaxWofford replied Dec 5, 2018

to clarify this was changed by rubocop's auto-format

@MaxWofford

This comment has been minimized.

Member

MaxWofford commented Dec 5, 2018

@lachlanjc: I figure it makes sense to add 'total click count' as a field on workshop projects

@zachlatta

This comment has been minimized.

Member

zachlatta commented Dec 5, 2018

Hey, so I still don't see all of my comments addressed. Can you let me know once you've addressed everything? I've dismissed the things I see addressed.

@MaxWofford

This comment has been minimized.

Member

MaxWofford commented Dec 5, 2018

@zachlatta sure you're looking at latest? the open comments were all outdated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment