[landing] Add `SubscriptionForm` to `HeroContent`
ClosedPublic
Actions

Authored by atul on Mar 10 2022, 12:25 PM.

Details

Reviewers

Commits

Summary

Add SubscriptionForm to HeroContent to make it more visible on the landing page.

Next will get rid of the Request Access button in the Header

Test Plan

Checked some other breakpoints to make sure things look good/proportional

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

atul published this revision for review.Mar 10 2022, 12:28 PM

atul added inline comments.

landing/hero-content.css
24 ↗	(On Diff #10277)	Also picked this value arbitrarily based on experimentation/checking what looked good. Is there like a more idiomatic way to do this? Maybe like proportional units or something? cc: @benschac
27–29 ↗	(On Diff #10277)	I picked this value arbitrarily based on experimentation/checking what looked good. Open to hearing if there's a better approach?
36–38 ↗	(On Diff #10277)	Just moved this into the other `.sub_heading` block

tomek requested changes to this revision.Mar 11 2022, 5:15 AM

tomek added inline comments.

landing/hero-content.css
24 ↗	(On Diff #10277)	This could be also achieved by using flex. In this case, the container should take some space (`min-height`) and would have `justify-content: space-between`. This approach would require having cycling-header and sub-heading in one div. But in this case margin sounds ok to me. (we're usually preferring padding, so that's also an option).
27–29 ↗	(On Diff #10277)	We can consider using flex for this. In this approach, we would have a flex container which displays children in a row. Then `form` would have e.g. `flex: 92 0` and there would be a spacer that displays nothing and have e.g. `flex: 8 1` which would mean that if there's enough space for the whole form to be displayed, the form should take 92% of the width. If the space is limited, then the spacer would shrink first. Just a general ideal - maybe some adjustments would be necessary.

This revision now requires changes to proceed.Mar 11 2022, 5:15 AM

• benschac added inline comments.

landing/hero-content.css
24 ↗	(On Diff #10277)	Also to @palys-swm -- prefer padding. I'd generally go for half of the heading unit as a scale. So, 1x would be half the font size. 2x the initial font size, and so on. I'd also say that this heading is generally in a weird place trying to get the cycling headers to not break the layout and not to worry about it.
27–29 ↗	(On Diff #10277)	You can set the initial size of the element with flex basis, rather than setting a relative width: https://developer.mozilla.org/en-US/docs/Web/CSS/flex-basis rather than setting a relative width. Like @palys-swm you can then adjust how the element grows or shrinks relative to it's other flex siblings in the flex container.

Didn't mean to resign, just wanted to leave comments.

rebase

landing/hero-content.css
24 ↗	(On Diff #10277)	But in this case margin sounds ok to me Okay, that seems simplest so I'll stick with that. Thanks for the suggestions though
27–29 ↗	(On Diff #10277)	Then form would have e.g. flex: 92 0 and there would be a spacer that displays nothing and have e.g. flex: 8 1 which would mean that if there's enough space for the whole form to be displayed, the form should take 92% of the width. That approach makes sense, but seems like it would give us the same end result with a little extra complexity? Thoughts on keeping it as is?

Accepting, as this behavior is good enough

landing/hero-content.css
27–29 ↗	(On Diff #10277)	The behavior would be different. Now we either take 92% or 100%. With flex, when we're in the state where 100% can't fit, we would shrink linearly up to 92%. So if e.g. only 95% would fit, then form would be 95% and spacer the remaining 5%. Usually it is a big improvement, but it's hard to say in this case, as we change the layout at 1100px.