Skip to content

Fix Unnecessary Margin style.scss#53

Merged
apoelstra merged 1 commit intoBlockstreamResearch:masterfrom
iajhff:master
Sep 19, 2025
Merged

Fix Unnecessary Margin style.scss#53
apoelstra merged 1 commit intoBlockstreamResearch:masterfrom
iajhff:master

Conversation

@iajhff
Copy link
Contributor

@iajhff iajhff commented Sep 11, 2025

1 │.main-content {
2 │ padding: 0 37px;
3 │- margin: 220px auto 75px;
4 │+ margin: 40px auto 75px;
5 │ @media (max-width: 768px) {
6 │- margin-top: 75px;
7 │+ margin-top: 30px;
8 │ padding: 0 10px;
9 │ }
10 │}

 1 │.main-content {
     2 │    padding: 0 37px;
     3 │-    margin: 220px auto 75px;
     4 │+    margin: 40px auto 75px;
     5 │    @media (max-width: 768px) {
     6 │-        margin-top: 75px;
     7 │+        margin-top: 30px;
     8 │        padding: 0 10px;
     9 │    }
    10 │}
@apoelstra
Copy link
Contributor

I have no idea how to review this. Is the padding "unnecessary"? According to whom? Where does this padding appear? How does it display on different devices?

Certainly, I need more justification than the word "unnecessary".

@iajhff
Copy link
Contributor Author

iajhff commented Sep 11, 2025

Apologies, I will improve my commit messages thank you for your guidance

If you visit: https://ide.simplicity-lang.org/ there is an extra margin-top for both max width and < 768px which takes up space between navigation and main-content which seems "unnecessary"

Reduces margin-top for .main-content >768px to 40px, and <768px as defined by @media property for responsiveness to 30px (save 10px for smaller width).


Existing live site:
Simplicity-IDE(1)
Simplicity-IDE


Applying to live version:
Screenshot 2025-09-11 at 12 15 34 PM
Screenshot 2025-09-11 at 12 15 27 PM


Locally deployed with full cargo build using scss instead of browser edit:
Screenshot 2025-09-11 at 12 10 57 PM
Screenshot 2025-09-11 at 12 10 48 PM

@apoelstra
Copy link
Contributor

Oops, sorry, I had meant to ACK and merge this last week.

Can you remove the new commit (see discussion in #54 -- I don't think it's correct)?

@iajhff
Copy link
Contributor Author

iajhff commented Sep 18, 2025

OK, I've reverted my master so it's only the one file changed. Thanks hope that this works fine for you

Copy link
Contributor

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK ead08c5; successfully ran local tests

@apoelstra apoelstra merged commit 41f193b into BlockstreamResearch:master Sep 19, 2025
4 checks 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.

2 participants