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

feat(herobody): add first pass of layout #1473

Merged
merged 4 commits into from
Sep 20, 2023

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Sep 5, 2023

Problem

we need to add new layout options for hero banner

Solution

  • Add panels on the hero section first
  • changed hero body so that it contains the new options now

Screenshots

Recording 2023-09-08 at 14 39 45

Tests

  • go to homepage
  • click on the hero section
  • select the various new layouts
  • the panel should reflect the new options
  • the preview should not work

@seaerchin seaerchin temporarily deployed to staging September 5, 2023 11:19 — with GitHub Actions Inactive
@seaerchin seaerchin force-pushed the feat/is-450-hero-banner-panel branch from 591fa75 to 81d9bf2 Compare September 7, 2023 16:29
@seaerchin seaerchin temporarily deployed to staging September 7, 2023 16:43 — with GitHub Actions Inactive
@seaerchin seaerchin requested a review from a team September 8, 2023 06:41
@seaerchin seaerchin marked this pull request as ready for review September 8, 2023 06:41
@seaerchin
Copy link
Contributor Author

ps, missing 1 title/subtitle on side layuot - adding now

@seaerchin seaerchin temporarily deployed to staging September 8, 2023 09:09 — with GitHub Actions Inactive
@seaerchin seaerchin force-pushed the feat/is-450-hero-banner-panel branch from af1bad8 to 2686bb0 Compare September 8, 2023 09:18
@seaerchin seaerchin temporarily deployed to staging September 8, 2023 09:34 — with GitHub Actions Inactive
Comment on lines +382 to +374
size="half"
alignment="left"
backgroundColor="black"
Copy link
Contributor

Choose a reason for hiding this comment

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

This resets the values every time the user selects a different option then comes back - are we okay with this behaviour?

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 is gonna be fixed in an upcoming PR - we're gonna roll out the new stuff totally (behind a feature flag) so this intermediate state won't be seen

alignment = "left",
backgroundColor = "black",
}: HeroSideSectionProps) => {
const [, setSectionSize] = useState(size)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need the set state without declaring the state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ps, i only added the panel - this means that the preview stuff here isn't working (it's a stop gap).

the actual preview component for these are gonna be added in a follow up and htis will be removed

Copy link
Contributor

Choose a reason for hiding this comment

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

ah okay

<Box w="100%">
<Text textStyle="subhead-1">Section background colour</Text>
<HStack spacing="0.75rem" alignItems="flex-start">
<IconButton
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe can store as [{}] and .map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ps, could you go into a bit more detail her?

Copy link
Contributor

Choose a reason for hiding this comment

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

thisis a nit: I see we are doing 3 times.

Instead can define the properties above as array of objects and then map over it. Might be neater

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will put as a function cos it relies on the icon colour (black/gray/white)

@seaerchin seaerchin requested review from a team, harishv7 and alexanderleegs September 19, 2023 10:03
@seaerchin seaerchin temporarily deployed to staging September 19, 2023 10:15 — with GitHub Actions Inactive
@seaerchin seaerchin force-pushed the feat/is-450-hero-banner-panel branch from d55f14e to 4ce3229 Compare September 20, 2023 04:40
@seaerchin seaerchin temporarily deployed to staging September 20, 2023 04:53 — with GitHub Actions Inactive
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 merged commit 9411888 into develop Sep 20, 2023
@seaerchin seaerchin deleted the feat/is-450-hero-banner-panel branch September 20, 2023 08:46
@harishv7 harishv7 mentioned this pull request Sep 20, 2023
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.

3 participants