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.

Sign up for GitHub

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

Jump to bottom

feat(homepage): add previews for side layout #1490

Merged
merged 13 commits into from
Sep 20, 2023
Merged

feat(homepage): add previews for side layout #1490

merged 13 commits into from
Sep 20, 2023

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Sep 13, 2023 •
edited
Loading

Problem

Previously, homepage did not have any previews for the hero side layout. This PR adds the previews in.

Solution

  1. add template component from preview
  2. add css styling for template component
    • this is a stop gap measure cos this PR came out before the PR for helpers so i'll have a PR post-merge to update. (easier to do then)

Screenshots

Recording 2023-09-20 at 12 23 52

Tests

  • go to homepage
  • select side variant
  • the preview should update with default options
  • select the various options
  • the preview should update correctly
  • update to either highlight/dropdown for hero interaction
  • preview should update correctly

@seaerchin seaerchin force-pushed the feat/homepage-phase-2 branch from 5817af5 to a5a18cd Compare September 13, 2023 06:21
@seaerchin seaerchin temporarily deployed to staging September 13, 2023 06:35 — with GitHub Actions Inactive
@seaerchin seaerchin temporarily deployed to staging September 13, 2023 07:09 — with GitHub Actions Inactive
@seaerchin seaerchin temporarily deployed to staging September 13, 2023 07:11 — with GitHub Actions Inactive
@seaerchin seaerchin force-pushed the feat/homepage-phase-2 branch from 5e6737e to 8df3ba8 Compare September 13, 2023 08:55
@seaerchin seaerchin temporarily deployed to staging September 13, 2023 09:08 — with GitHub Actions Inactive
@seaerchin seaerchin requested a review from a team September 15, 2023 08:38
@seaerchin seaerchin marked this pull request as draft September 15, 2023 08:39
@seaerchin seaerchin force-pushed the feat/homepage-phase-2 branch from 8df3ba8 to 26cc73d Compare September 20, 2023 04:17
@seaerchin seaerchin marked this pull request as ready for review September 20, 2023 04:22
@seaerchin seaerchin temporarily deployed to staging September 20, 2023 04:30 — with GitHub Actions Inactive
@seaerchin seaerchin force-pushed the feat/homepage-phase-2 branch from 26cc73d to 76c0b19 Compare September 20, 2023 05:14
@seaerchin seaerchin temporarily deployed to staging September 20, 2023 05:29 — with GitHub Actions Inactive
alexanderleegs
alexanderleegs reviewed Sep 20, 2023
View reviewed changes
Copy link
Contributor

@alexanderleegs alexanderleegs left a comment

Choose a reason for hiding this comment

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

Just to confirm, is dropdown not enabled for side section layout? Should we be disabling the button?

src/layouts/components/Homepage/HeroBody.tsx Outdated Show resolved Hide resolved
src/layouts/components/Homepage/HeroBody.tsx Outdated Show resolved Hide resolved
src/templates/homepage/HeroSection/HeroSection.tsx Show resolved Hide resolved
dcshzj
dcshzj reviewed Sep 20, 2023
View reviewed changes
src/styles/templates/theme/margin.scss
@@ -0,0 +1,7 @@
.mb4 {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should shift to use the padding and margin helpers for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was raised prior to the helpers... i don't want to block so i'll probably do as a follow-up; is that ok with you?

src/templates/homepage/HeroSection/HeroSideLayout.tsx Outdated Show resolved Hide resolved
src/templates/homepage/HeroSection/HeroSideLayout.tsx
Comment on lines +135 to +144
style={{
paddingTop: "106px",
paddingBottom: "106px",
paddingLeft: "84px",
paddingRight: "84px",
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this seems very specific, are we able to use any of the padding helpers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above - good point and you're absolutely right but i'll do it in another PR

@seaerchin
Copy link
Contributor Author

Just to confirm, is dropdown not enabled for side section layout? Should we be disabling the button?

the dropdown PRs (#1494 and #1491) are already approved - this is just an intermediate PR to ease reviewing load

@seaerchin seaerchin requested review from alexanderleegs, dcshzj and a team September 20, 2023 07:48
dcshzj
dcshzj approved these changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@dcshzj dcshzj left a comment

Choose a reason for hiding this comment

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

Shucks I meant for all my comments to be done in a separate PR, psps

alexanderleegs
alexanderleegs approved these changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@alexanderleegs alexanderleegs left a comment

Choose a reason for hiding this comment

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

lgtm

@seaerchin seaerchin temporarily deployed to staging September 20, 2023 08:01 — with GitHub Actions Inactive
Base automatically changed from feat/is-451-preview to develop September 20, 2023 08:49
@seaerchin seaerchin force-pushed the feat/homepage-phase-2 branch from a6b1f7c to a0ea13b Compare September 20, 2023 08:55
@seaerchin seaerchin merged commit de687ad into develop Sep 20, 2023
3 checks passed
@seaerchin seaerchin deleted the feat/homepage-phase-2 branch September 20, 2023 08:57
@seaerchin seaerchin temporarily deployed to staging September 20, 2023 09:10 — with GitHub Actions Inactive
@harishv7 harishv7 mentioned this pull request Sep 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcshzj dcshzj dcshzj approved these changes

@alexanderleegs alexanderleegs alexanderleegs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seaerchin @alexanderleegs @dcshzj