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

[Bugfix:Forum] Fix Forum Post Spacing #10176

Merged
merged 5 commits into from
Feb 16, 2024
Merged

[Bugfix:Forum] Fix Forum Post Spacing #10176

merged 5 commits into from
Feb 16, 2024

Conversation

hansongu123
Copy link
Member

closes #10070

Please check if the PR fulfills these requirements:

  • Screenshots are attached to Github PR if visual/UI changes were made

What is the current behavior?

#10118 fixes the first part of #10070 by containing the post text in a block, but this way it adds extra spacing on top and bottom of a post to match that of a markdown enabled post, causing there to be more wasted space.

What is the new behavior?

This PR adds whitespace control to the non-markdown enabled text and removes the extra spacing for the posts w/ markdown.
A 5px top-margin is added to improve the look and feel.

Before:
image

Now:
image

Copy link

codecov bot commented Feb 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (350ffae) 22.79% compared to head (84b6eae) 22.79%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main   #10176   +/-   ##
=========================================
  Coverage     22.79%   22.79%           
  Complexity     8352     8352           
=========================================
  Files           228      228           
  Lines         29907    29907           
  Branches         75       75           
=========================================
  Hits           6817     6817           
  Misses        23018    23018           
  Partials         72       72           
Flag Coverage Δ
autograder 21.92% <ø> (ø)
js 27.09% <ø> (ø)
migrator 100.00% <ø> (ø)
php 19.71% <ø> (ø)
python_submitty_utils 71.65% <ø> (ø)
submitty_daemon_jobs 91.01% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@Kush-Raval Kush-Raval left a comment

Choose a reason for hiding this comment

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

I did a functionality test and there didn't seem to be any glitches. I tested with different size screens and nothing broke the general function of the page.
The code changes make sense as to how you get the desired result.
Looks good to merge

Copy link
Contributor

@jeffrey-asm jeffrey-asm left a comment

Choose a reason for hiding this comment

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

The fix looks good on my end. Works as intended for single lines and multiple paragraphs. I would change the selector to "post-content" to fix the CSS Lint issues. I would also look into removing ".post_content p:first-child/last-child" because there is no paragraph tag within any of the "post_content" containers.

@hansongu123 hansongu123 marked this pull request as draft February 12, 2024 16:06
@hansongu123 hansongu123 marked this pull request as ready for review February 14, 2024 01:01
@hansongu123
Copy link
Member Author

hansongu123 commented Feb 14, 2024

@jeffreyC4 there's p block created for every paragraph if the post is markdown enabled due to how it is rendered, and each paragraph get's the same top and bottom margining hence we want to remove them for the first and last paragraphs. With disabling css lint for these two lines there should be no more failing checks.

@Kush-Raval @jeffreyC4 guys can you re-review and make sure the white space control (- -) doesn't affect other things?

Copy link
Contributor

@jeffrey-asm jeffrey-asm left a comment

Choose a reason for hiding this comment

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

Everything looks great. The spacing is no longer an issue for regular and markdown posts on the forum. I tried various markdown comments, but they all seemed fine.

Copy link
Contributor

@Kush-Raval Kush-Raval left a comment

Choose a reason for hiding this comment

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

Ran this on my local machine and saw the desired effects. Messed with different screen sizes and formats and the change still worked. Drastic improvement on space inside posts.
Tested as instructor and student.
The code makes sense and I can see why it will result in the desired effect.
Ready to merge.

@bmcutler bmcutler merged commit 85e68d5 into main Feb 16, 2024
18 checks passed
@bmcutler bmcutler deleted the forum_post_spacing branch February 16, 2024 14:38
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.

Markdown posts have padding that non-markdown posts do not
4 participants