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

Styling #29

Merged
merged 13 commits into from
Jul 8, 2024
Merged

Styling #29

merged 13 commits into from
Jul 8, 2024

Conversation

paulmckissock
Copy link
Contributor

@paulmckissock paulmckissock commented Jul 3, 2024

Styled article pages, comment pages, user pages, login page, signup page, reset password, user article pages, and new article page.

Copy link
Contributor

@noahko96 noahko96 left a 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.

app/views/articles/new.html.erb Show resolved Hide resolved
app/views/articles/show.html.erb Outdated Show resolved Hide resolved
app/views/users/comments.html.erb Outdated Show resolved Hide resolved
@noahko96 noahko96 requested a review from nathanlong July 3, 2024 19:57
app/helpers/comments_helper.rb Show resolved Hide resolved
app/helpers/comments_helper.rb Show resolved Hide resolved
app/helpers/comments_helper.rb Outdated Show resolved Hide resolved
app/helpers/comments_helper.rb Outdated Show resolved Hide resolved
…s to comments. Replaced many reoccuring inline styles with css classes.
Copy link
Contributor

@noahko96 noahko96 left a 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;
}
Copy link
Contributor

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 on lines +74 to +92
.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 {
Copy link
Contributor

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
Copy link
Contributor

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| %>
Copy link
Contributor

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| %>
Copy link
Contributor

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;">
Copy link
Contributor

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.

@paulmckissock paulmckissock merged commit f1dd44e into main Jul 8, 2024
1 check passed
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.

3 participants