www
/
www-new
12
3
Fork 5
Code Issues 58 Pull Requests 42 Projects Releases Wiki Activity

Make padding-bottom consistent for pages with EmailSignup component #247

Merged
j285he merged 8 commits from jared-fix-padding-bottom-101 into main 2021-09-02 17:35:43 -04:00
Conversation 5 Commits 8 Files Changed 5 +21 -5
j285he commented 2021-08-31 23:47:56 -04:00
Member
Copy Link

Closes #101.

Closes #101.
j285he added 1 commit 2021-08-31 23:47:57 -04:00
continuous-integration/drone/push Build is passing Details
69d0568832 Make padding consistent
j285he added 1 commit 2021-08-31 23:49:04 -04:00
continuous-integration/drone/push Build is passing Details
88bb11bf32 em -> rem
j285he added 1 commit 2021-08-31 23:49:45 -04:00
continuous-integration/drone/push Build is passing Details
08d3dff085 em -> rem 2: Electric Boogaloo
j285he commented 2021-08-31 23:50:36 -04:00
Author
Member
Copy Link

Might it be worth looking into a uniform padding-bottom added to global styles?

edit: or margin-bottom, if that is what is preferred

Might it be worth looking into a uniform padding-bottom added to global styles? edit: or margin-bottom, if that is what is preferred
j285he requested review from a3thakra 2021-08-31 23:52:20 -04:00
a258wang commented 2021-09-01 12:13:39 -04:00
Owner
Copy Link

Looks good to me! Just a couple of suggestions/notes:

  • Maybe we can adjust the bottom padding/margin on the Our Supporters page as well? It feels like it has less space down there than all the other pages.
  • I'm not sure whether we should be using padding or margin in this context. Perhaps someone more knowledgeable can shed some light on this?
Looks good to me! Just a couple of suggestions/notes: - Maybe we can adjust the bottom padding/margin on the Our Supporters page as well? It feels like it has less space down there than all the other pages. - I'm not sure whether we should be using padding or margin in this context. Perhaps someone more knowledgeable can shed some light on this?
👍 1
j285he added 1 commit 2021-09-01 17:01:24 -04:00
continuous-integration/drone/push Build is passing Details
441eb67770 Add padding to our-supporters
j285he commented 2021-09-01 17:05:01 -04:00
Author
Member
Copy Link

My thought process was that padding is a part of an object, whereas margin is used to separate 2 different object. Since to me the space at the bottom felt like an integral part of .page, I used padding. For example, if we were to add a border to .page (it would be very ugly if we did), then I think it makes sense that there is space between the border and content at the bottom.

I could be way off the mark though, let me know what you guys think!

My thought process was that padding is a part of an object, whereas margin is used to separate 2 different object. Since to me the space at the bottom felt like an integral part of `.page`, I used padding. For example, if we were to add a border to `.page` (it would be very ugly if we did), then I think it makes sense that there is space between the border and content at the bottom. I could be way off the mark though, let me know what you guys think!
👍 1
b38peng approved these changes 2021-09-01 20:54:53 -04:00
b38peng commented 2021-09-01 20:55:23 -04:00
Member
Copy Link

looks good to me! Just need to pull from main 👍

looks good to me! Just need to pull from main 👍
👍 1
j285he added 1 commit 2021-09-01 21:44:48 -04:00
continuous-integration/drone/push Build is passing Details
6cb86f0e81 Merge branch 'main' into jared-fix-padding-bottom-101
a258wang closed this pull request 2021-09-02 01:03:17 -04:00
a258wang reopened this pull request 2021-09-02 01:03:21 -04:00
a3thakra commented 2021-09-02 12:21:19 -04:00
Owner
Copy Link

I think arguments can be made on both sides (margin vs padding).

The main technical difference is that margins are collapsible - https://medium.com/@joseph0crick/margin-collapse-in-css-what-why-and-how-328c10e37ca0

I prefer margin over padding for that reason, but I'll leave the decision up to you @j285he

The PR LGTM overall, so feel free to merge.

I think arguments can be made on both sides (margin vs padding). The main technical difference is that margins are collapsible - https://medium.com/@joseph0crick/margin-collapse-in-css-what-why-and-how-328c10e37ca0 I prefer margin over padding for that reason, but I'll leave the decision up to you @j285he The PR LGTM overall, so feel free to merge.
j285he added 2 commits 2021-09-02 17:23:56 -04:00
j285he added 1 commit 2021-09-02 17:26:08 -04:00
continuous-integration/drone/push Build is passing Details
8b3154cc2e add margin to about
j285he merged commit 423f97f37b into main 2021-09-02 17:35:43 -04:00
a3thakra reviewed 2021-09-04 20:43:28 -04:00
pages/get-involved.module.css
@ -1,5 +1,5 @@
.page {
margin: calc(50rem / 16) 0;
margin: calc(60rem / 16) 0;
a3thakra commented 2021-09-04 20:43:28 -04:00
Owner
Copy Link

This is changing the margins on the side as well, which make the page super narrow.

This is changing the margins on the side as well, which make the page super narrow.
a258wang reviewed 2021-09-04 21:02:19 -04:00
pages/get-involved.module.css
@ -52,2 +52,4 @@
@media only screen and (max-width: calc(768rem / 16)) {
.page {
margin: calc(30rem / 16);
a258wang commented 2021-09-04 21:02:19 -04:00
Owner
Copy Link

Continuing from Adi's comment: this should be margin: calc(30rem / 16) 0 so we don't change the side margins.

Continuing from Adi's comment: this should be `margin: calc(30rem / 16) 0` so we don't change the side margins.
a3thakra deleted branch jared-fix-padding-bottom-101 2021-09-04 21:42:44 -04:00
Sign in to join this conversation.
Reviewers
No reviewers
a3thakra
b38peng
Labels
Clear labels   
a11y

   
Backlog

   
Blocked

   
Bug

   
Content

   
Dependencies

   
Design

   
Feature Request

   
Good First Issue

   
In Progress

   
Performance

   
Priority - High

   
Priority - Low

   
Priority - Medium

   
Untriaged
No Label
a11y
Backlog
Blocked
Bug
Content
Dependencies
Design
Feature Request
Good First Issue
In Progress
Performance
Priority - High
Priority - Low
Priority - Medium
Untriaged
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: www/www-new#247
No description provided.
Delete Branch "jared-fix-padding-bottom-101"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?