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.

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

Upload image with paperclip and usage postgres database #5

Merged
merged 33 commits into from
Sep 12, 2018

Conversation

buitoancnth
Copy link
Owner

No description provided.

def create
@album = current_user.albums.new album_params
if @album.save
if params[:images]

Choose a reason for hiding this comment

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

Nếu tạo album thành công mà không có hình ảnh nào hết thì sao? Khi đó code của em chưa thấy xử lý trường hợp đó

def my_albums
@albums = current_user.albums.page params[:page]
render :index
# render :index

Choose a reason for hiding this comment

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

Dư code bị comment

@@ -2,6 +2,6 @@ class FeedsController < ApplicationController
layout 'layout_album_photo'

def index
redirect_to photos_path
# redirect_to photos_path

Choose a reason for hiding this comment

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

Dư code bị comment

render :new
end
end

def my_photos
@photos = current_user.photos.page params[:page]
# render :index

Choose a reason for hiding this comment

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

Dư commented code

@@ -1,30 +1,31 @@
class SessionsController < Devise::SessionsController
class SessionsController < Devise::SessionsController

Choose a reason for hiding this comment

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

Có cần phải override file SessionsController hay không? Tại sao cần phải override?

Copy link
Owner Author

@buitoancnth buitoancnth Sep 6, 2018

Choose a reason for hiding this comment

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

em override vì sau này khi đăng nhập xong e thêm flash với lại render hay redirect_to , mà em chỉ mới thêm vào chưa xử lý.

Choose a reason for hiding this comment

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

Tại sao em cần thêm Flash với lại render? Có yêu cầu gì đặc biệt hay không?

<%= f.text_area :description, class: 'form-control' %>
<%= f.label :image %>
<%= f.file_field :image %>
<%= f.submit %>

Choose a reason for hiding this comment

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

Button cần có class của Bootstrap

@@ -1,3 +1,5 @@
<%= button_to "Add Photo", { action: "new" }, method: :get, class: 'btn-primary upload' %>

Choose a reason for hiding this comment

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

Thiếu class btn

<% end %>
</ul>
<% end %>
<%= f.label :title %>

Choose a reason for hiding this comment

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

Form như thế này là chưa đúng kiểu của bootstrap, cần xem trên trang Bootstrap form tạo ra có những thẻ gì, class như thế nào và làm tương ứng. Ví dụ <div class="form-group">

<% end %>
</ul>
<% end %>
<%= f.label :title %>

Choose a reason for hiding this comment

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

Form như thế này là chưa đúng kiểu của bootstrap, cần xem trên trang Bootstrap form tạo ra có những thẻ gì, class như thế nào và làm tương ứng. Ví dụ <div class="form-group">

@@ -0,0 +1,9 @@
class ChangeColumnPhoto < ActiveRecord::Migration[5.1]
def up
change_column :photos, :album_id, :integer

Choose a reason for hiding this comment

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

Tại sao up và down đều giống như nhau? Nếu vậy thì migration này dùng để làm gì?

Copy link
Owner Author

Choose a reason for hiding this comment

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

cái này bữa e test dùng để set cho khóa ngoại bằng 0 để add data, e xóa cái thuộc tính trong def down rồi. file này không cần thiết a.

@@ -0,0 +1,8 @@
<%= link_to new_album_path do %>
<button type="button" class="btn btn-primary">Add Album</button>

Choose a reason for hiding this comment

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

Chưa dùng i18n

<hr>
<%= form_for @album, html: { multipart: true } do |f| %>
<% if @album.errors.any? %>
<h3>Some errors were found:</h3>

Choose a reason for hiding this comment

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

Chưa dùng i18n

@@ -1,3 +1,7 @@
<%= link_to new_photo_path do %>
<button type="button" class="btn btn-primary">Add Photo</button>

Choose a reason for hiding this comment

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

Chưa dùng i18n

@@ -0,0 +1,29 @@
<h3 class="text-primary">New Photo</h3>

Choose a reason for hiding this comment

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

Chưa dùng i18n

<hr>
<%= form_for @photo do |f| %>
<% if @photo.errors.any? %>
<h3>Some errors were found:</h3>

Choose a reason for hiding this comment

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

Chưa dùng i18n

.navbar {
background-color: blue;
.nav > li.dropdown > a {
color: #fff;

Choose a reason for hiding this comment

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

Indentation lộn xộn quá

@@ -8,25 +8,42 @@
font-size: 1.7em;
color: #fff;
text-transform: uppercase;
padding-top: 9px;
align-content: center;

Choose a reason for hiding this comment

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

Đây là thuộc tính dùng cho flex, e có đang dùng đúng không vậy? https://www.w3schools.com/cssref/css3_pr_align-content.asp

text-align: center;
width: 100%;
display: inline;
}
div.caption p {

Choose a reason for hiding this comment

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

Chưa tận dụng được sức mạnh của SCSS. Em có thể khai báo như sau

div.caption {
   p {
   }

  span {
  }
}

top: 75%;
left: 84%;
}
i {

Choose a reason for hiding this comment

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

Em override nguyên thẻ luôn sao? Tại sao lại làm như vậy? Thẻ i thì nó là để in nghiêng, không được thay đổi các thuộc tính của nó như vậy

@@ -1,5 +1,6 @@
class AlbumsController < ApplicationController
layout 'layout_album_photo', only: :index
before_action :load_image, only: :create

Choose a reason for hiding this comment

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

Hàm của em đặt tên không đúng, theo nội dung a đọc thì hàm đó e dùng để check có params[:images] được gửi lên server hay không, cần rename hàm cho đúng

top: 5px;
font-size: 15px;
}
span:hover {

Choose a reason for hiding this comment

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

Cái này có thể để ngay trong thẻ span ở trên, như sau

span {
   &:hover{
   }
}


def check_image
@images = params[:album][:images]
return if @images

Choose a reason for hiding this comment

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

Logic này hơi khó hiểu cho người đọc, nếu là check coi có params image hay không thì nên viết logic như sau

unless @images
   redirect_to new_album_path
end

def destroy
@photo.destroy
redirect_to my_photos_path
end
def my_photos

Choose a reason for hiding this comment

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

Cần phân tách 2 methods bằng một dòng trống

has_attached_file :image, styles: { thumb: ["200x150#", :png] }
validates_attachment :image, content_type: { content_type: /\Aimage\/.*\z/ }, presence: true
scope :order_by_created_at, -> { order(created_at: :desc) }
scope :photo_pucblic, -> { where(share_mode: true) }

Choose a reason for hiding this comment

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

Lỗi chính tả pucblic. Và scope này đã nằm trong model Photo nên không cần phải có chữ photo_ ở đầu nữa, chỉ cần đặt scope :public là được

@@ -2,20 +2,17 @@
<% albums.each do |album| %>
<div class="col-md-6">
<div class="post-image">
<div class="pop"><%= image_tag("image2.jpg",size: "200x200") %></div>
<div class="pop"><%= image_tag("image2.jpg",size: "200x200", class: '') %></div>

Choose a reason for hiding this comment

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

Class rỗng để làm gì? Nếu không có class thì không cần chỉ định gì cả

<% else %>
<li><%= link_to "Log in", new_user_session_path %></li>
<li><%= link_to "Log in", new_user_session_path %></li>

Choose a reason for hiding this comment

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

Chưa dùng i18n

<div class="post-data">
<%= image_tag("image2.jpg", class: 'avatar') %>
<a href="#" class="post-catagory"><%= User.find(photo.user_id).fullname %></a>
<h5 class="post-name text-primary" ><%= User.find(photo.user_id).fullname %></h5>

Choose a reason for hiding this comment

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

Không được dùng User.find như thế này trong view, cần dùng association

</div>
<span class="post-like glyphicon glyphicon-heart"></span>
<span class="number-like">123</span>
<span class="post-date"><%= Time.zone.at(photo.created_at).strftime("%I:%M %p %m/%d/%Y") %></span>

Choose a reason for hiding this comment

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

Đưa hàm format này vào 1 helper, chỗ khác cũng dùng lại được. Khi cần format thì chỉ cần gọi như sau

<%= format_time(photo.created_at) %>

</div>
<% if index == 1 %>

Choose a reason for hiding this comment

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

Logic khúc này nữa là sao? Tại sao nếu index là 1 thì sẽ có các thẻ div như vậy?

<% end %>
</div>
</div>
<% if index == 3 %>

Choose a reason for hiding this comment

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

Logic này để làm gì?


def index
@albums = Album.page params[:page]
@albums = Album.album_public.order_by_created_at.page params[:page]

Choose a reason for hiding this comment

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

scope trong album thì không cần prefix 'album_' nữa


def destroy
@album.destroy
flash[:success] = "deleted success !"

Choose a reason for hiding this comment

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

Chưa có i18n cho message

validates :title, length: { maximum: 140 }, presence: true, if: :check_album_id?
validates :description, length: { maximum: 300 }, presence: true, if: :check_album_id?
scope :order_by_created_at, -> { order(created_at: :desc) }
scope :photo_public, -> { where(share_mode: true) }

Choose a reason for hiding this comment

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

Không cần để tiền tố là photo_ trong scope này, chỉ cần để public là được

validates :title, length: { maximum: 140 }, presence: true
validates :description, length: { maximum: 300 }, presence: true
scope :order_by_created_at, -> { order(created_at: :desc) }
scope :album_public, -> { where(share_mode: true) }

Choose a reason for hiding this comment

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

Không cần để album_ trong scope này

scope :order_by_created_at, -> { order(created_at: :desc) }
scope :photo_public, -> { where(share_mode: true) }

def check_album_id?
Copy link

@xuanchien xuanchien Sep 10, 2018

Choose a reason for hiding this comment

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

Nên đặt tên hàm là no_album? hoặc album_is_empty?

<div class="thumbnail">
<% if album.photos.present? %>
<% album.photos.order_by_created_at.each_with_index do |image, index| %>
<% break if index == 3 %>

Choose a reason for hiding this comment

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

Mục đích đoạn if này là gì?

Copy link
Owner Author

Choose a reason for hiding this comment

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

số 3 này là em mún cho cái album chỉ show tối đa 3 ảnh trong cái album có số ảnh phía trong ấy a
để e đặt lại biến constant

</div>
<div class="form-group">
<%= f.label :last_name %>
<%= f.text_field :last_name, autofocus: true, placeholder: t('.last_name'), class: 'form-control' %>

Choose a reason for hiding this comment

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

Dư autofocus

</div>
<div class="form-group">
<%= f.label :email %>
<%= f.email_field :email, autofocus: true, placeholder: t('.email'), class: 'form-control' %>

Choose a reason for hiding this comment

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

Dư autofocus

</ul>
</li>
<li>
<%= link_to t('logout'), destroy_user_session_path, method: :delete %>

Choose a reason for hiding this comment

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

Nên dùng lazy lookup thay vì đặt key logout ở level 1 như trong file locales.

Copy link
Owner Author

Choose a reason for hiding this comment

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

có mấy cái chung em hay dùng em để ở mục ngoài cùng không biết đc không anh

Choose a reason for hiding this comment

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

Cũng được nếu như em đã hiểu khi nào cần lazy lookup, khi nào không

<% else %>
<li><%= link_to "Log in", new_user_session_path %></li>
<li><%= link_to t('login'), new_user_session_path %></li>

Choose a reason for hiding this comment

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

Nên dùng lazy lookup thay vì đặt key login ở level 1 như trong file locales.

@@ -1,3 +1,4 @@
<%= csrf_meta_tags %>
<meta name="viewport" content="width=device-width, initial-scale=1.0"></meta>

Choose a reason for hiding this comment

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

A có xem lại thì thẻ sẽ không cần thẻ đóng, em remove thẻ đóng đi

<%= f.password_field :password_confirmation, autocomplete: "off", class: 'form-control' %>
</div>
<div class="form-group">
<%= f.label :current_password %> <i>(we need your current password to confirm your changes)</i><br />

Choose a reason for hiding this comment

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

Chưa dùng i18n

<div class="form-group">
<%= f.label :password_confirmation %>
<% if @minimum_password_length %>
<em>(<%= @minimum_password_length %> characters minimum) </em>

Choose a reason for hiding this comment

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

Chưa dùng i18n

<% end %>
</div>
<div class="form-group">
<%= f.label :password %> <i>(leave blank if you don't want to change it)</i><br />

Choose a reason for hiding this comment

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

Chưa dùng i18n

</div>
<div class="form-group">
<% if devise_mapping.confirmable? && resource.pending_reconfirmation? %>
<div>Currently waiting confirmation for: <%= resource.unconfirmed_email %></div>

Choose a reason for hiding this comment

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

Chưa dùng i18n


def update_images
self.photos.delete_all
@images.each do |image|

Choose a reason for hiding this comment

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

Nên gọi hàm create_images bên trong này thay vì viết lại logic tạo images

@buitoancnth buitoancnth merged commit 9800326 into master Sep 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants