-
Notifications
You must be signed in to change notification settings - Fork 0
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
Styling #29
Styling #29
Conversation
… application.html.erb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small things from me. I'm also going to have a UI dev glance over this just to be sure.
…s to comments. Replaced many reoccuring inline styles with css classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments on things that could be improved, but will leave that up to you if you actually want to make those changes since we are sort've moving on from this project.
.comment { | ||
max-width: 1215px; | ||
overflow-wrap: anywhere; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be tabbed in.
.comment { | ||
max-width: 1215px; | ||
overflow-wrap: anywhere; | ||
} | ||
.post-subtext { | ||
height:5px; | ||
} | ||
.header-space { | ||
height:10px | ||
} | ||
.page-table { | ||
border:0; | ||
border-collapse: collapse; | ||
} | ||
.page-table td, | ||
.page-table th { | ||
padding: 0; | ||
} | ||
.paginate-spacing { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put line breaks between all of these blocks to clean them up.
if comment.replies.any? | ||
concat(content_tag(:ul) do | ||
concat(content_tag(:ul, style: "padding-left: 20px;") do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would replace this with a class instead of the inline styling.
<div><%= message %></div> | ||
<% end %> | ||
</div> | ||
<%= form_with model: @article do |form| %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you made article
a helper method, I would use it here instead of @article
.
<tr class="header-space"></tr> | ||
<tr> | ||
<td colspan="3"> | ||
<%= form_with(model: [@article, @comment], local: true) do |form| %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also use the helper method here.
</tr> | ||
<tr> | ||
<td colspan="3"> | ||
<ul style="list-style-type: none; padding-left: 0;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more inline styling I would replace.
Styled article pages, comment pages, user pages, login page, signup page, reset password, user article pages, and new article page.