Models #3
Models #3
Conversation
There was a problem hiding this comment.
Status: Approved 🟢
Hi @HFG43
Great work on implementing the new feature, this is complete so it's time to merge it 💯 🥳.
Highlights 🥇
- UI Design looks beautiful and clean
- The models, controllers, and views have correct logic
- Good use of join_table
Optional
All the changes with optional tags are not crucial enough to prevent you from getting the approval.
As I mentioned before, all the designs match the ones provided for this project, but still there are some things you can do to improve it. Please remember this is completely optional and you don't need to make it.
Happy coding! 👏 👏 👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear. Remember to tag me in your question so I can receive the notification.
app/views/pages/welcome.html.erb
Outdated
| <% else %> | ||
| <%= link_to 'Sign In', new_user_session_path %> |
There was a problem hiding this comment.
@HFG43 EXCELLENT JOB creating the SplashScreen styles, the UI is beautiful, and everything looks very clean and organized, however, I would like to leave a suggestion that is completely optional but I think it could help to have a design more similar to the original. I kindly recommend that you give this button a different background color, specifically the blue color that you are using as a hover. It would also be better if the Sign In button is called Log In
Example of Log In button background color:
app/views/layouts/_navbar.html.erb
Outdated
|
|
||
| <%unless current_page?(groups_path)%> | ||
| <%= image_tag 'right_arrow.svg', class: 'arrow'%> | ||
| <% end %> |
There was a problem hiding this comment.
@HFG43 I suggest you also place the Sign out button here when the user is logged in. This is very positive at the user experience level since the user will be able to have the option visible and available at all times without having to go to the root page for that.
| <div class="category_data_container"> | ||
| <p class="reg_text_col"><%= group.name %></p> | ||
| <% group.expenses.each do |spend|%> | ||
| <%@group_amount += spend.amount%> | ||
| <%end%> | ||
| <p class="reg_text_col_bold"><%=@group_amount%></p> | ||
| </div> | ||
| <div class="category_data_date reg_text_col"> | ||
| <%=group.created_at.strftime("%d %b %Y")%> | ||
| </div> |
There was a problem hiding this comment.
@HFG43 [OPTIONAL] I also suggest increasing the font size here a little more so that the information is easier to read, especially for users who may have vision problems. This will make your application much more accessible.
Example: For the group name and amount, I think you could increase the font size by at least 5 pixels more. For the date, I think you could increase the font size by at least 3 pixels more.
In the end, you can choose a new size that you think is best.
I also recommend increasing the size of the images, by at least 20px more pixels in both width and height.
|
|
||
| <% @expenses.each do |expense|%> | ||
| <div class="category_data_container disp_col"> | ||
| <h3><p class="reg_text_col_bold"><%="TRANSACTION N° #{expense.id}"%></h3> | ||
| <div class="category_data_date reg_text_col transaction_disp"> | ||
| <p><%= expense.created_at.strftime("%d %b %Y at %l%P")%></p> | ||
| <span class="reg_text_col_bold"><%= expense.amount%></span> | ||
| </div> | ||
| </div> | ||
| <%end%> |
There was a problem hiding this comment.
@HFG43 [OPTIONAL] I also suggest increasing the font size here a little more.
| <%= image_tag @group.icon, class: 'icon' %> | ||
| </div> | ||
| <%= @group.name%> | ||
| </div> |
There was a problem hiding this comment.
@HFG43 [OPTIONAL] I also suggest increasing the font size here a little more.
Example: For the group name, I think you could increase the font size by at least 5 pixels more.
| <div class="bottom_button_container"> | ||
| <%= link_to "ADD NEW TRANSACTION", new_group_expense_path(@group), class: 'bottom_button' %> | ||
| </div> |
There was a problem hiding this comment.
@HFG43 I also recommend increasing the width of the button (link_to) for the mobile version. I suggest you at least 70% (In CSS = width: 70%) so that it fits a little more into the original layout
| <div class="bottom_button_container"> | ||
| <%= form.submit "ADD NEW CATEGORY", class: 'bottom_button' %> | ||
| </div> |
There was a problem hiding this comment.
@HFG43 I also recommend increasing the width of the button (link_to) for the mobile version. I suggest you at least 70% (In CSS = width: 70%) so that it fits a little more into the original layout
app/views/layouts/_navbar.html.erb
Outdated
| <% end %> | ||
| <p><%= "#{@path_description}"%></p> | ||
|
|
There was a problem hiding this comment.
@HFG43 [OPTIONAL] I also suggest increasing the font size here a little more so that the page title will look better and easier to read
| <%= f.submit "Log in" %> | ||
| <div class="sign_up_button_container"> | ||
| <%= f.submit "Log in", class: "sign_up_button" %> | ||
| </div> |
There was a problem hiding this comment.
@HFG43 I kindly suggest you add custom styles for the button and give a little more space between the form fields and the button.
| <div class="actions"> | ||
| <%= f.submit "Sign up" %> | ||
| <div class="sign_up_button_container"> | ||
| <p><%= f.submit "Sign up", class: "sign_up_button" %></p> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
@HFG43 I kindly suggest you add custom styles for the button and give a little more space between the form fields and the button.
Hi there!! I'm sending this pull request for you to review my code. Thanks in advance for your feedback!
Milestones 🎯:
📌 Add Style including all views
📌 Add links and back buttons when required
Please consider some commands and links should be added in the upcoming views and controllers
🎖️ General requirements:
Gitflow
No linter errors
Ensure the use of best practices and Ruby style