Skip to content

Conversation

nicpicks
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

Fixes alignment issue for divs appearing outside home container

Related Tickets & Documents

#4379

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

alt_text

@nicpicks nicpicks requested a review from a team October 15, 2019 07:13
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 15, 2019
@ghost ghost requested review from jessleenyc and removed request for a team October 15, 2019 07:13
@rhymes
Copy link
Contributor

rhymes commented Oct 15, 2019

Hi @wei-tg, thanks for your PR! I was wondering how did you find this? It's awesome!

Did you manage to reproduce this alignment problem?

@nicpicks
Copy link
Contributor Author

nicpicks commented Oct 15, 2019

Hi @rhymes, I did a DOM comparison between a working page and a page with problems mentioned in the issue. This helped narrow down to where it broke, which was in div class="comments-container-container".
The broken page had the class show-comments-footer outside of the comments-container-container when it should have been inside. Taking a look at the file itself, it would make sense that it was not properly enclosed causing an extra div.

@CLAassistant
Copy link

CLAassistant commented Oct 17, 2019

CLA assistant check
All committers have signed the CLA.

@rhymes
Copy link
Contributor

rhymes commented Oct 17, 2019

@wei-tg can you please sign the CLA? thanks!

@nicpicks
Copy link
Contributor Author

done so :)

Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

I think there's an issue with the styling now that the alignment is fixed.

This is how it looks on master:

Screenshot_2019-10-17  I Will Fear No Evil

This is how it looks on this branch:

Screenshot_2019-10-17  I Will Fear No Evil(1)

I was looking the page of an article with comments on my localhost

@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 17, 2019
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Oct 18, 2019
@nicpicks
Copy link
Contributor Author

Okay, I think the problem was when full_comments_area was refactored to use a partial view. It needs a parent container, so by moving the </div> to wrap the entire partial view, it worked because now <div id="comments"> became the parent container.

I traced where this code first originated to 76afb43
and I feel a container like this <div id="comments" style="margin-top: -50px;padding-bottom:50px;position:relative;z-index:-100"></div> served no meaning, so might as well make it the parent container for _full_comments_area.html.erb

@rhymes
Copy link
Contributor

rhymes commented Oct 18, 2019

@wei-tg great investigative skills!

Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Tested with Firefox 70 and Chrome 77, works great!

Thanks again for the PR @wei-tg :)

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 18, 2019
Copy link
Contributor

@maestromac maestromac left a comment

Choose a reason for hiding this comment

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

Awesome work @wei-tg !! LGTM!

@maestromac maestromac merged commit 0b05289 into forem:master Oct 22, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: merged bot applied label for PR's that are merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants