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

♻️ Removed extra space between rule content and Giscus #1669

Merged
merged 9 commits into from
Jan 24, 2025

Conversation

babakamyljanovssw
Copy link
Member

@babakamyljanovssw babakamyljanovssw commented Jan 15, 2025

As per email from Adam: "Giscus space".

Changes:
This space is not from Giscus container, instead there is <section> container with no data inside between rule content and Giscus which was rendering extra space with thin line.
As per my conversation with @tiagov8, kept thin line for better UI, and reduced the top and bottom margin.

image
Figure: Empty container which was rendering space

image
Figure: ❌ Before - there is extra space after rule content and Giscus

image
Figure: ✅ After - the space is reduced and thin line was kept between rule content and Giscus

Copy link
Member

@tiagov8 tiagov8 left a comment

Choose a reason for hiding this comment

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

Hi @babakamyljanovssw

You don't need to create a class for this.
See https://www.ssw.com.au/rules/avoid-unnecessary-css-classes/

  1. Please remove the new class you created
  2. Change the existing Tailwind "mb-2" class to be "mb-6" (we'll get the same result 😉)

@babakamyljanovssw
Copy link
Member Author

Hi @tiagov8,

Removed custom margin styling and used ml-2 and mr-2.
Kept break-line class because it was giving thin line styling: border-top: 1px solid #f5f5f5;

image

Copy link
Member

@tiagov8 tiagov8 left a comment

Choose a reason for hiding this comment

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

Hey @babakamyljanovssw

We don't need the "break-line" class either

  1. Please replace the whole section with a <hr /> (same as we have at the top of rules)
Screenshot 2025-01-21 at 12 14 43 PM

This way we don't need any extra classes or CSS changes

@babakamyljanovssw
Copy link
Member Author

babakamyljanovssw commented Jan 22, 2025

Hi @tiagov8

  1. Please replace the whole section with a <hr /> (same as we have at the top of rules)

✅ Done

image

tiagov8
tiagov8 previously approved these changes Jan 22, 2025
@babakamyljanovssw babakamyljanovssw merged commit 9f63a44 into main Jan 24, 2025
5 checks passed
@babakamyljanovssw babakamyljanovssw deleted the remove-space branch January 24, 2025 02:12
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