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

Improve code quality and readability in /app/views/shared/_stats.html.erb #968

Closed
julianguyen opened this Issue Sep 18, 2018 · 22 comments

Comments

Projects
None yet
6 participants
@julianguyen
Member

julianguyen commented Sep 18, 2018

Description

When working on implementing the redesigns I noticed that /app/views/shared/_stats.html.erb was pretty messy to look at (did not have time to clean it up myself). There's a lot of logic in this view when it should be moved to a helper and possibly subviews.

@imi56

This comment has been minimized.

imi56 commented Oct 3, 2018

I would love to work on this.

@julianguyen

This comment has been minimized.

Member

julianguyen commented Oct 3, 2018

Hey @aSquare14, if you don't mind I'm going to assign you and @prateksha from this issue just because you two have multiple open PRs that still need to be wrapped up. Happy to sync with you after if you want to work on a refactoring related issue.

@julianguyen

This comment has been minimized.

Member

julianguyen commented Oct 3, 2018

Hey @imi56, thanks for expressing your interest. Please check out https://github.com/ifmeorg/ifme/wiki/Join-Our-Slack for next steps :) After that, assign yourself to the issue.

@OPARA-PROSPER

This comment has been minimized.

OPARA-PROSPER commented Oct 6, 2018

@julianguyen Can i work on this issue?

@member-cop

This comment has been minimized.

member-cop bot commented Oct 6, 2018

Hey @OPARA-PROSPER, thanks for expressing your interest. We would love your help with this issue. Please follow these next steps to join our contributor community.

@julianguyen

This comment has been minimized.

Member

julianguyen commented Oct 6, 2018

@OPARA-PROSPER Of course! Go ahead and assign yourself to the issue :)

@julianguyen julianguyen removed this from To Do in RSGOC 2018: Team Rubies Oct 6, 2018

@OPARA-PROSPER OPARA-PROSPER self-assigned this Oct 6, 2018

@OPARA-PROSPER

This comment has been minimized.

OPARA-PROSPER commented Oct 7, 2018

Can you suggest how you'd love to improve the code quality @julianguyen, the codebase is quite readable to me (though am new to ruby and rails).

What i did:

  • I compared the codebase with existing view codebase to see how others were structured and it looked normal to me.
@member-cop

This comment has been minimized.

member-cop bot commented Oct 7, 2018

Hey @OPARA-PROSPER, thanks for expressing your interest. We would love your help with this issue. Please follow these next steps to join our contributor community.

@julianguyen

This comment has been minimized.

Member

julianguyen commented Oct 8, 2018

@OPARA-PROSPER Good question, so that file is quite lengthy for a view file. We could start off by moving lines 20-42 to another view partial.

There's some conditional logic that is repeated throughout the file, so you could move it into a helper for e.g. !local_assigns[:profile] || local_assigns[:profile] == current_user.id

@julianguyen

This comment has been minimized.

Member

julianguyen commented Oct 8, 2018

I would also look for opportunities to refactor code e.g.

  • Replace uses of where with find
  • Replace local_assigns[:data_type] == 'category' || local_assigns[:data_type] == 'mood' || local_assigns[:data_type] == 'strategy' with %w(category mood strategy).include? local_assigns[:data_type]
@OPARA-PROSPER

This comment has been minimized.

OPARA-PROSPER commented Oct 8, 2018

@julianguyen Thanks for the feedback, i really appreciate them!

Can you give me a little bit of time to wrap my head around this concepts, i'd need to dig deep because i'd love to understand what i'm doing better, so i can learn better.

if i eventually venture into ruby (Which probably looks so), that will be great for me. <3

@member-cop

This comment has been minimized.

member-cop bot commented Oct 8, 2018

Hey @OPARA-PROSPER, thanks for expressing your interest. We would love your help with this issue. Please follow these next steps to join our contributor community.

@julianguyen

This comment has been minimized.

Member

julianguyen commented Oct 8, 2018

@ScotianMan ^ any ideas why this bot keeps posting? 🙃

@julianguyen

This comment has been minimized.

Member

julianguyen commented Oct 8, 2018

@OPARA-PROSPER Of course, take your time and feel free to ask questions on Slack too.

Here are some articles I found on refactoring Rails views which may be helpful learning material:
https://brandonhilkert.com/blog/refactoring-logic-from-a-rails-view/
https://allenan.com/refactoring-rails-views/
https://medium.com/@scottm/refactoring-views-with-ruby-on-rails-activesupport-helpers-7d8b71c81ce2

@OPARA-PROSPER

This comment has been minimized.

OPARA-PROSPER commented Oct 8, 2018

They look great @julianguyen . Thank you

@member-cop

This comment has been minimized.

member-cop bot commented Oct 8, 2018

Hey @OPARA-PROSPER, thanks for expressing your interest. We would love your help with this issue. Please follow these next steps to join our contributor community.

@ScotianMan

This comment has been minimized.

Contributor

ScotianMan commented Oct 10, 2018

looking into it now @julianguyen, sorry I was away for thxGiving 🦃

@ScotianMan

This comment has been minimized.

Contributor

ScotianMan commented Oct 10, 2018

should be fixed, sorry about that @julianguyen

@julianguyen

This comment has been minimized.

Member

julianguyen commented Oct 10, 2018

No worries, I hope you had a good one! Thanks again @ScotianMan! :D

@OPARA-PROSPER

This comment has been minimized.

OPARA-PROSPER commented Oct 11, 2018

@julianguyen i submitted a PR for this issue

@member-cop

This comment has been minimized.

member-cop bot commented Oct 11, 2018

Hey @OPARA-PROSPER, thanks for expressing your interest. We would love your help with this issue. Please follow these next steps to join our contributor community.

@ScotianMan

This comment has been minimized.

Contributor

ScotianMan commented Oct 11, 2018

I wonder why its only posting in this one case... debugging now

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